De-zigzag JPEG's DQT when loading; deprecate convert_dict_qtables #4989
Conversation
Re-order the JPEG quantization tables to normal order when loading. This wastes a few CPU cycles if you don't need them. But it has the advantage of hiding the zigzag order JPEG implementation detail of these tables completely from Pillow users. This difference has led to cases where: * arrays in zigzag order were taken from a dict and passed directly as a qtables parameter, causing them to be "zigzagged" again by libjpeg. * dicts with lists in normal order being passed to JpegImagePlugin.convert_dict_qtables, causing them to be unnecessarily "de-zigzagged".
| "convert_dict_qtables is deprecated and will be removed in a future" | ||
| " release. Conversion is no longer needed.", | ||
| DeprecationWarning, | ||
| ) |
There was a problem hiding this comment.
A thought (feel free to disagree) - if we're considering this to be a public method that requires deprecation, then should we maybe just leave the functionality there (still adding in the deprecation warning)?
There was a problem hiding this comment.
I searched GitHub for this function: https://github.com/search?q=convert_dict_qtables+language%3Apython+-filename%3AJpegPresets.py. There are not many uses. Only the first two pages contain interesting results. The rest is just copies of Pillow itself. They all take the .quantization of an existing image, convert it to a nested list with this function and then manipulate that nested list in some way.
If Pillow de-zigzags the tables when loading (as the rest of this PR does), .quantization no longer needs any conversion step. That means those existing uses of the convert_dict_qtables function will actually break their code. The order of the numbers in the tables will be wrong. They'd need to remove the function call to fix their code. Turning convert_dict_qtables into a no-op means their code will continue to work unchanged. They'll only need to remove the function call to get rid of the warning.
Most people don't need to mess with quantization tables. But there's obviously more code than just what I found on GitHub and I don't know how representative these uses are. I guess there could also be cases where people are getting qtables from somewhere other than straight from an image's .quantization. Maybe they serialized them for later re-use? In that case they no longer have a helper function that converts them back.
I guess Pillow could temporarily add a new really_convert_dict_qtables for those who still need it. 😉
There was a problem hiding this comment.
@radarhere What do you think? Shall we merge this PR as is?
(And can explicitly replace "a future version" with "Pillow 10 (2023-01-02)")
There was a problem hiding this comment.
Sure - I can see now why convert_dict_qtables should do nothing, so no objections from me.
|
Let's also add it to https://pillow.readthedocs.io/en/stable/deprecations.html |
|
Thanks also for fixing the order of the other deprecations in the docs! |
Re-order the JPEG quantization tables to normal order when loading. This wastes a few CPU cycles if you don't need them.
But it has the advantage of hiding the zigzag order JPEG implementation detail of these tables completely from Pillow users.
This difference has led to cases where:
again by libjpeg.
Fixes #4962
Changes proposed in this pull request: