Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.4k
Ensure TIFF RowsPerStrip is multiple of 8 for JPEG compression#5588
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kmilos commentedJul 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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): |
kmilos commentedJul 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks, that leaves YCbCr mode TIFF saving which it seems never worked w/ JPEG compression? I also tried setting |
kmilos commentedJul 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 clearbug 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. |
| rows_per_strip=min((2**16+stride-1)//stride,im.size[1]) | ||
| # JPEG encoder expects multiple of 8 rows | ||
| ifcompression=="jpeg": | ||
| rows_per_strip=min(((rows_per_strip+7)//8)*8,im.size[1]) |
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.
| rows_per_strip=min(((rows_per_strip+7)//8)*8,im.size[1]) | |
| rows_per_strip=((rows_per_strip+7)//8)*8 |
Becauserows_per_strip has already been set to beim.size[1] at minimum, and the calculation here only ever rounds up, themin part is unnecessary.
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 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.
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.
Ok, thanks
radarhereJul 10, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
| 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?
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.
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.
Fixes#5586