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.
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
Initialize rows_per_strip when RowsPerStrip tag is missing #4034
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 usingTiffStripSize
would give room for a better result, although in testing I find that it does just give back theysize
. So at the moment, I'm inclined to go back to @cgohlke's version.There was a problem hiding this comment.
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 useTIFFStripSize
. Forrows_per_strip
it would be possible to useTIFFGetFieldDefaulted
.something like that :