Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Initialize rows_per_strip when RowsPerStrip tag is missing #4034

Merged
merged 3 commits into from Sep 30, 2019
Merged

Initialize rows_per_strip when RowsPerStrip tag is missing #4034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b37f12a
Initialize rows_per_strip when RowsPerStrip tag is missing
cgohlke Aug 20, 2019
19ab3c3
Added test
radarhere Sep 18, 2019
f5aed1a
Merge branch 'master' into patch-1
hugovk Sep 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Binary file added Tests/images/no_rows_per_strip.tif
View file
Open in desktop
Binary file not shown.
7 changes: 7 additions & 0 deletions Tests/test_file_libtiff.py
View file
Open in desktop
Expand Up @@ -832,6 +832,13 @@ def test_old_style_jpeg(self):
im, "Tests/images/old-style-jpeg-compression.png"
)

def test_no_rows_per_strip(self):
# This image does not have a RowsPerStrip TIFF tag
infile = "Tests/images/no_rows_per_strip.tif"
im = Image.open(infile)
im.load()
self.assertEqual(im.size, (950, 975))

def test_orientation(self):
base_im = Image.open("Tests/images/g4_orientation_1.tif")

Expand Down
6 changes: 5 additions & 1 deletion src/libImaging/TiffDecode.c
View file
Open in desktop
Expand Up @@ -405,8 +405,12 @@ int ImagingLibTiffDecode(Imaging im, ImagingCodecState state, UINT8* buffer, Py_
UINT32 strip_row, row_byte_size;
UINT8 *new_data;
UINT32 rows_per_strip;
int ret;

TIFFGetField(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip);
ret = TIFFGetField(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip);
if (ret != 1) {
rows_per_strip = state->ysize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why set this to state->ysize?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check, but assumed that state->ysize is the number of rows in the image. No?

According to the TIFF6 specification on the RowsPerStrip tag: "The default is 2**32 - 1, which is effectively infinity. That is, the entire image is one strip". That is the case here. One could also try to estimate rows_per_strip from the number of strips and the RowsPerStrip tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative, I've created a PR to use TIFFStripSize instead, as mentioned in the comment.

Pillow/src/libImaging/TiffDecode.c

Line 412 in 2eb447e

// We could use TIFFStripSize, but for YCbCr data it returns subsampled data size

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkopachev did you have any thoughts on my version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@radarhere My only concern is YCbCr TIFFs.
However I see that in case of used subsampling, RowsPerStrip must be an integer multiple of YCbCrSubsampleVert so we should be good here.

But I'd add a comment about that in the code near by.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other option would be to use TIFFStripSize to calculate byte size of a strip. Account for subsampling as well if that's the case.
Then we can use TIFFGetFieldDefaulted(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip). And since it won't be used for calculating bytesize of a strip, then it should be all good.

That might look like cleaner and more explicit approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite following sorry.

Is using TIFFStripSize to calculate the byte size of a strip not exactly what I'm doing in my updated version (not the Outdated version Github is showing that we're commenting on)?

Using TIFFGetFieldDefaulted goes back to the idea of using the default, the entire image as one strip. I had hoped that using TiffStripSize would give room for a better result, although in testing I find that it does just give back the ysize. So at the moment, I'm inclined to go back to @cgohlke's version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I was referring to case when you don't need to calculate rows_per_strip manually when use TIFFStripSize. For rows_per_strip it would be possible to use TIFFGetFieldDefaulted.

something like that :

TIFFGetFieldDefaulted(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip);
state->bytes = TIFFStripSize(tiff);
if (TIFF_IS_YCBCR) {
  row_byte_size = (state->xsize * state->bits + 7) / 8;
  state->bytes = rows_per_strip * row_byte_size;
}

}
TRACE(("RowsPerStrip: %u \n", rows_per_strip));

// We could use TIFFStripSize, but for YCbCr data it returns subsampled data size
Expand Down