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

De-zigzag JPEG's DQT when loading; deprecate convert_dict_qtables #4989

Merged
merged 7 commits into from Jun 30, 2021

Conversation

@gofr
Copy link
Contributor

@gofr gofr commented Oct 17, 2020

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

Fixes #4962

Changes proposed in this pull request:

  • De-zigzag JPEG quantization tables when reading from file, causing all internal formats to consistently represent them in normal array order.
  • Make JpegImagePlugin.convert_dict_qtables a deprecated no-op.
gofr added 2 commits Oct 17, 2020
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,
)
Copy link
Member

@radarhere radarhere Apr 15, 2021

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)?

Copy link
Contributor Author

@gofr gofr Apr 15, 2021

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

Copy link
Member

@hugovk hugovk Jun 28, 2021

@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)")

Copy link
Member

@radarhere radarhere Jun 29, 2021

Sure - I can see now why convert_dict_qtables should do nothing, so no objections from me.

@radarhere radarhere added the JPEG label Jun 29, 2021
Tests/test_file_jpeg.py Outdated Show resolved Hide resolved
src/PIL/JpegImagePlugin.py Outdated Show resolved Hide resolved
hugovk
hugovk approved these changes Jun 30, 2021
@hugovk
Copy link
Member

@hugovk hugovk commented Jun 30, 2021

@radarhere radarhere force-pushed the 4962-jpeg-zigzag branch from 602e3c2 to 375e112 Jun 30, 2021
@hugovk
Copy link
Member

@hugovk hugovk commented Jun 30, 2021

Thanks also for fixing the order of the other deprecations in the docs!

@mergify mergify bot merged commit 9149509 into python-pillow:master Jun 30, 2021
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants