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

Prevent masking of Image reduce method in Jpeg2KImagePlugin #4474

Merged
merged 4 commits into from
Mar 29, 2020

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Mar 14, 2020

Resolves #4343. Alternative to #4344

The new Image reduce method does not work for Jpeg2K images, as Jpeg2KImagePlugin has a reduce variable that masks it. The variable is not purely internal, but is actually tested behaviour -

def test_reduce(self):
with Image.open("Tests/images/test-card-lossless.jp2") as im:
im.reduce = 2
im.load()
self.assertEqual(im.size, (160, 120))

PR #4344 tried to address this by treating the setting of reduce as an integer, and the use of reduce as a method, as two different scenarios that were not to interact.

So instead, this PR changes Pillow so that if you set reduce as an integer, the user will then see reduce as an integer.

with Image.open("Tests/images/test-card-lossless.jp2") as im:
        assert callable(im.reduce)

        im.reduce = 2
        assert im.reduce == 2

At that point, you can't call the reduce method externally. If Pillow tries to call the reduce method internally though, it will ignore this new use of reduce, and still call the Pillow method.

src/PIL/Image.py Show resolved Hide resolved
src/PIL/Jpeg2KImagePlugin.py Show resolved Hide resolved
src/PIL/Jpeg2KImagePlugin.py Outdated Show resolved Hide resolved
Copy link
Member

@homm homm left a comment

Choose a reason for hiding this comment

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

Like this

src/PIL/Jpeg2KImagePlugin.py Outdated Show resolved Hide resolved
src/PIL/Jpeg2KImagePlugin.py Outdated Show resolved Hide resolved
@python-pillow python-pillow deleted a comment from codecov bot Mar 20, 2020
@python-pillow python-pillow deleted a comment from codecov bot Mar 20, 2020
@python-pillow python-pillow deleted a comment from codecov bot Mar 20, 2020
@radarhere
Copy link
Member Author

@homm so you approve of this PR then?

@homm
Copy link
Member

homm commented Mar 20, 2020

What about deprecation warnings?

@homm
Copy link
Member

homm commented Mar 24, 2020

What about deprecation warnings?

Otherwise is ok.

@radarhere
Copy link
Member Author

Ok, I've added a deprecation warning pointing users to use load_reduce instead.

@homm
Copy link
Member

homm commented Mar 24, 2020

I believe, load_reduce is not the way how it should work. There is already .draft() method exactly for this functionality, which should be used.

@radarhere
Copy link
Member Author

Correct me if I'm wrong - there isn't a draft method for Jpeg2kImagePlugin. It only exists for JpegImagePlugin.

@homm
Copy link
Member

homm commented Mar 25, 2020

Draft method exists for PIL.Image.Image. It "configures the image file loader so it returns a version of the image that as closely as possible matches the given mode and size". This is exactly what Jpeg2KImageFile.reduce was erroneously used for.

To be clear: I don't mean you have to implement Jpeg2KImageFile.draft() method to fix this bug, I just strongly against adding extra API for things that already exist.

@radarhere
Copy link
Member Author

Sure, sure. I'm just confused about the idea of adding a deprecation warning without being able to tell them what they should be doing instead.

I've removed load_reduce and the deprecation warning. How do you feel about merging this in for 7.1.0, to resolve #4343 and then maybe we introduce a deprecation warning and a Jpeg2K method at a later date?

@python-pillow python-pillow deleted a comment from codecov bot Mar 29, 2020
This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JPEG 2000 file cannot be Image.reduce() 'd
2 participants