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

Ensure TIFF RowsPerStrip is multiple of 8 for JPEG compression #5588

Merged
merged 2 commits into from Jul 18, 2021

Conversation

@kmilos
Copy link
Contributor

@kmilos kmilos commented Jul 7, 2021

Fixes #5586

@radarhere radarhere added the TIFF label Jul 7, 2021
@kmilos
Copy link
Contributor Author

@kmilos kmilos commented Jul 7, 2021

This works only for RGB mode 4:4:4 data (no subsampling). AFAICT, we don't support YCbCr subsampling directly when saving to TIFF, but the tag might be copied over from another info structure, so might be an idea to handle it?

However, there's something weird going on wrt to saving YCbCr mode instead of RGB, even if using single strip (i.e. 8.2.0):

OSError: encoder error -9 when writing image file

junk.tiff: Warning, fractional scanline discarded.
JPEGLib: Application transferred too few scanlines.

@radarhere radarhere mentioned this pull request Jul 7, 2021
@kmilos
Copy link
Contributor Author

@kmilos kmilos commented Jul 7, 2021

Thanks, that leaves YCbCr mode TIFF saving which it seems never worked w/ JPEG compression?

I also tried setting ifd[530] = (1,1) (YCbCr subsampling to 4:4:4) as the default is (2,2) which doesn't correspond to the data, but the encoder doesn't like that - I think unfortunately TIFFSetField interface is different for this tag (2 ints) compared to the expected count+pointer (e.g. ExtraSamples)? Setting atts[530] = 1 didn't work either.

@kmilos
Copy link
Contributor Author

@kmilos kmilos commented Jul 7, 2021

YCbCr saving doesn't even work uncompressed (i.e. no libtiff involved), even if I add correct tags 530, 531, and 532. I guess this is a separate issue.

Uncompressed YCbCr 4:4:4 seems to work after all (if correct tags 530 and 532 are added). Just that most viewers don't seem to support it...

However, there is a clear bug w/ libtiff_encoder, as it tries to save tag 530 2-tuple with a "default" type LONG (i.e. count+offset), which doesn't fit into the expected 4 bytes value/offset sub-field as 2 SHORTs would.

@@ -1577,6 +1577,9 @@ def _save(im, fp, filename):
# aim for 64 KB strips when using libtiff writer
if libtiff:
rows_per_strip = min((2 ** 16 + stride - 1) // stride, im.size[1])
# JPEG encoder expects multiple of 8 rows
if compression == "jpeg":
rows_per_strip = min(((rows_per_strip + 7) // 8) * 8, im.size[1])
Copy link
Member

@radarhere radarhere Jul 10, 2021

Suggested change
rows_per_strip = min(((rows_per_strip + 7) // 8) * 8, im.size[1])
rows_per_strip = ((rows_per_strip + 7) // 8) * 8

Because rows_per_strip has already been set to be im.size[1] at minimum, and the calculation here only ever rounds up, the min part is unnecessary.

Copy link
Contributor Author

@kmilos kmilos Jul 10, 2021

I believe it is needed, precisely because it rounds up. This is to support the rare case of small images where im.size[1]<8 and you end up as a single strip. Libtiff/jpeg codec have no problem handling the single/last strip that is not factor of 8, as it is padded internally.

Copy link
Member

@radarhere radarhere Jul 10, 2021

Ok, thanks

Copy link
Member

@radarhere radarhere Jul 10, 2021

Suggested change
rows_per_strip = min(((rows_per_strip + 7) // 8) * 8, im.size[1])
rows_per_strip = min(((rows_per_strip + 7) // 8) * 8, rows_per_strip)

What about this?

Copy link
Contributor Author

@kmilos kmilos Jul 11, 2021

That won't work for the corner case where you have u multi-strip w/ e.g. rows_per_strip=2 and im.size[1]=6. For JPEG, you want to end up as single strip of 6 rows.

@radarhere radarhere merged commit 7484bb0 into python-pillow:master Jul 18, 2021
50 checks passed
@kmilos kmilos deleted the patch-2 branch Jul 18, 2021
@radarhere radarhere mentioned this pull request Aug 27, 2021
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants