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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions Tests/test_file_jpeg.py
Expand Up @@ -446,7 +446,7 @@ def _n_qtables_helper(n, test_file):
assert len(im.quantization) == n
reloaded = self.roundtrip(im, qtables="keep")
assert im.quantization == reloaded.quantization
assert reloaded.quantization[0].typecode == "B"
assert max(reloaded.quantization[0]) <= 255

with Image.open("Tests/images/hopper.jpg") as im:
qtables = im.quantization
Expand All @@ -458,7 +458,8 @@ def _n_qtables_helper(n, test_file):

# valid bounds for baseline qtable
bounds_qtable = [int(s) for s in ("255 1 " * 32).split(None)]
self.roundtrip(im, qtables=[bounds_qtable])
im2 = self.roundtrip(im, qtables=[bounds_qtable])
assert im2.quantization == {0: bounds_qtable}

# values from wizard.txt in jpeg9-a src package.
standard_l_qtable = [
Expand Down Expand Up @@ -569,6 +570,12 @@ def test_save_low_quality_baseline_qtables(self):
assert max(im2.quantization[0]) <= 255
assert max(im2.quantization[1]) <= 255

def test_convert_dict_qtables_deprecation(self):
with pytest.warns(DeprecationWarning):
qtable = {0: [1, 2, 3, 4]}
qtable2 = JpegImagePlugin.convert_dict_qtables(qtable)
assert qtable == qtable2

@pytest.mark.skipif(not djpeg_available(), reason="djpeg not available")
def test_load_djpeg(self):
with Image.open(TEST_FILE) as img:
Expand Down
14 changes: 9 additions & 5 deletions src/PIL/JpegImagePlugin.py
Expand Up @@ -252,7 +252,7 @@ def DQT(self, marker):
data = array.array("B" if precision == 1 else "H", s[1:qt_length])
if sys.byteorder == "little" and precision > 1:
data.byteswap() # the values are always big-endian
self.quantization[v & 15] = data
self.quantization[v & 15] = [data[i] for i in zigzag_index]
s = s[qt_length:]


Expand Down Expand Up @@ -585,9 +585,11 @@ def _getmp(self):


def convert_dict_qtables(qtables):
qtables = [qtables[key] for key in range(len(qtables)) if key in qtables]
for idx, table in enumerate(qtables):
qtables[idx] = [table[i] for i in zigzag_index]
warnings.warn(
"convert_dict_qtables is deprecated and will be removed in Pillow 10"
"(2023-01-02). Conversion is no longer needed.",
DeprecationWarning,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return qtables


Expand Down Expand Up @@ -668,7 +670,9 @@ def validate_qtables(qtables):
qtables = [lines[s : s + 64] for s in range(0, len(lines), 64)]
if isinstance(qtables, (tuple, list, dict)):
if isinstance(qtables, dict):
qtables = convert_dict_qtables(qtables)
qtables = [
qtables[key] for key in range(len(qtables)) if key in qtables
]
elif isinstance(qtables, tuple):
qtables = list(qtables)
if not (0 < len(qtables) < 5):
Expand Down
14 changes: 3 additions & 11 deletions src/PIL/JpegPresets.py
Expand Up @@ -52,19 +52,11 @@

im.quantization

This will return a dict with a number of arrays. You can pass this dict
This will return a dict with a number of lists. You can pass this dict
directly as the qtables argument when saving a JPEG.

The tables format between im.quantization and quantization in presets differ in
3 ways:

1. The base container of the preset is a list with sublists instead of dict.
dict[0] -> list[0], dict[1] -> list[1], ...
2. Each table in a preset is a list instead of an array.
3. The zigzag order is remove in the preset (needed by libjpeg >= 6a).

You can convert the dict format to the preset format with the
:func:`.JpegImagePlugin.convert_dict_qtables()` function.
The quantization table format in presets is a list with sublists. These formats
are interchangeable.

Libjpeg ref.:
https://web.archive.org/web/20120328125543/http://www.jpegcameras.com/libjpeg/libjpeg-3.html
Expand Down