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.

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 3 commits into from Sep 30, 2019

Conversation

@cgohlke
Copy link
Contributor

cgohlke commented Aug 20, 2019

@radarhere radarhere added the Bug label Aug 21, 2019
@radarhere radarhere added this to Backlog in Pillow Aug 21, 2019
@radarhere radarhere removed this from Backlog in Pillow Aug 21, 2019
TIFFGetField(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip);
ret = TIFFGetField(tiff, TIFFTAG_ROWSPERSTRIP, &rows_per_strip);
if (ret != 1) {
rows_per_strip = state->ysize;

This comment has been minimized.

Copy link
@radarhere

radarhere Aug 21, 2019

Member

Why set this to state->ysize?

This comment has been minimized.

Copy link
@cgohlke

cgohlke Aug 21, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@radarhere

radarhere Sep 18, 2019

Member

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

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

This comment has been minimized.

Copy link
@radarhere

radarhere Sep 26, 2019

Member

@kkopachev did you have any thoughts on my version?

This comment has been minimized.

Copy link
@kkopachev

kkopachev Sep 26, 2019

Contributor

@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.

This comment has been minimized.

Copy link
@kkopachev

kkopachev Sep 26, 2019

Contributor

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.

This comment has been minimized.

Copy link
@radarhere

radarhere Sep 29, 2019

Member

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.

This comment has been minimized.

Copy link
@kkopachev

kkopachev Sep 30, 2019

Contributor

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;
}
radarhere and others added 2 commits Sep 18, 2019
@hugovk
hugovk approved these changes Sep 29, 2019
@radarhere

This comment has been minimized.

Copy link
Member

radarhere commented Sep 30, 2019

I've reverted to using @cgohlke's version.

@radarhere radarhere merged commit fb84701 into python-pillow:master Sep 30, 2019
20 checks passed
20 checks passed
Python 3.7
Details
Python 3.7
Details
codecov/patch 100% of diff hit (target 85.5%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +14.49% compared to 152ed62
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
python-pillow.Pillow Build #20190930.4 succeeded
Details
python-pillow.Pillow (Lint Python37) Lint Python37 succeeded
Details
python-pillow.Pillow (alpine) alpine succeeded
Details
python-pillow.Pillow (amazon_1_amd64) amazon_1_amd64 succeeded
Details
python-pillow.Pillow (amazon_2_amd64) amazon_2_amd64 succeeded
Details
python-pillow.Pillow (arch) arch succeeded
Details
python-pillow.Pillow (centos_6_amd64) centos_6_amd64 succeeded
Details
python-pillow.Pillow (centos_7_amd64) centos_7_amd64 succeeded
Details
python-pillow.Pillow (debian_10_buster_x86) debian_10_buster_x86 succeeded
Details
python-pillow.Pillow (debian_9_stretch_x86) debian_9_stretch_x86 succeeded
Details
python-pillow.Pillow (fedora_29_amd64) fedora_29_amd64 succeeded
Details
python-pillow.Pillow (fedora_30_amd64) fedora_30_amd64 succeeded
Details
python-pillow.Pillow (ubuntu_16_04_xenial_amd64) ubuntu_16_04_xenial_amd64 succeeded
Details
python-pillow.Pillow (ubuntu_18_04_bionic_amd64) ubuntu_18_04_bionic_amd64 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Pillow
  
New Issues
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.