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

Add support for 16-bit precision JPEG quantization values #4918

Merged
merged 6 commits into from Oct 14, 2020
Merged

Add support for 16-bit precision JPEG quantization values #4918

merged 6 commits into from Oct 14, 2020

Conversation

gofr
Copy link
Contributor

@gofr gofr commented Sep 12, 2020

This only adds the support on the Python side. This won't really do anything without further changes to forward this to the C code.

This is a partial fix for #4825.

I've changed the TRUE to FALSE in

quality, TRUE);
to verify that this fixes the problem for the image attached to #4825.

To fully address the issue I guess the qtables property of the JPEGENCODERSTATE needs to be expanded to include the precision info so we can tell the encoder which format to use.

@gofr gofr marked this pull request as draft September 12, 2020 18:32
@radarhere radarhere added the JPEG label Sep 12, 2020
@gofr
Copy link
Contributor Author

gofr commented Sep 13, 2020

Do you think Pillow needs its own force_baseline option for compatibility? If not, just flipping the force_baseline flag from TRUE to FALSE in JpegEncode.c is actually enough.

These 16-bit precision quantization values are apparently something you'd normally only get in low quality JPEGs, and they are optional in the standard according to https://github.com/libjpeg-turbo/libjpeg-turbo/blob/6d8caa9f88184ad1698e91c94b37acb9506dacf0/usage.txt#L123 so some decoders may not support them.

Even with force_baseline=FALSE libjpeg will write 8-bit values if all the values are <= 255. So saved files would only have 16-bit values if someone uses quality='keep'/qtables='keep' and the input also had values that needed 16-bits, or someone specifically specified their own table(s) with values > 255.

libjpeg (and Pillow) will emit the JTRC_16BIT_TABLES warning ("Caution: quantization tables are too coarse for baseline JPEG") when the only thing that prevents a JPEG from being "baseline" is the 16-bit quantization values.

@gofr gofr marked this pull request as ready for review September 13, 2020 18:18
@radarhere
Copy link
Member

Are you able to add tests that fail without your changes and pass with them?

Don't force JPEG quantization to be baseline-compatible

Quantization values will not be limited to values 1..255 and may
be 16 bits if needed. This may cause compatibility issues.
16-bit quantization tables can appear at quality values below
about 25. These may cause compatibility problems. Maintain
baseline compatibility and avoid confusing users with warning
messages.
@gofr
Copy link
Contributor Author

gofr commented Oct 10, 2020

I can add tests...

@gofr
Copy link
Contributor Author

gofr commented Oct 10, 2020

I've made one change to my earlier implementation. When using the quality parameter Pillow will continue to use force_baseline=TRUE. This is a bit of an arbitrary decision. But it felt safer to ensure greater compatibility of JPEGs by default. (And to avoid the "Caution" warning I mentioned in my previous comment in cases where users wouldn't expect it.) This means quantization values will be forced to 8 bits. According to https://github.com/libjpeg-turbo/libjpeg-turbo/blob/master/wizard.txt:

At very low quality settings, some quantization table entries
can get scaled up to values exceeding 255. Although 2-byte quantization
values are supported by the IJG software, this feature is not in baseline
JPEG and is not supported by all implementations. If you need to ensure
wide compatibility of low-quality files, you can constrain the scaled
quantization values to no more than 255 by giving the -baseline switch.
Note that use of -baseline will result in poorer quality for the same file
size, since more bits than necessary are expended on higher AC coefficients.

So my fix will only create 16-bit values if you specify your own tables or if you use qtables='keep' and the original tables already had 16-bit values. That seems like a fair balance.

I also ran into #4962 while writing the tests. I can fix that one too if you agree that it needs fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants