Initialize rows_per_strip when RowsPerStrip tag is missing#4034
Initialize rows_per_strip when RowsPerStrip tag is missing#4034radarhere merged 3 commits intopython-pillow:masterfrom cgohlke:patch-1
Conversation
| TIFFGetField(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip); | ||
| ret = TIFFGetField(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip); | ||
| if (ret != 1) { | ||
| rows_per_strip = state->ysize; |
There was a problem hiding this comment.
Why set this to state->ysize?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
|
I've reverted to using @cgohlke's version. |
Fix segfault reported at https://stackoverflow.com/questions/57578369/multi-layer-tiff-import-broken-in-pillow-6-1-0. Please review.