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

Relaxed child images check to allow for libjpeg #6853

Merged
merged 1 commit into from Feb 12, 2023

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Jan 2, 2023

Reported in conda-forge/pillow-feedstock#128 (comment)

test_get_child_images() in test_file_jpeg.py is failing with libjpeg. This PR relaxes the check to test for a similarity of 2.1, instead of equality. I have confirmed this in pillow-wheels with libjpeg 9e.

@h-vetinari
Copy link

Sorry, this was a false alarm!

I got caught out by the fact that test_strip_ycbcr_jpeg_2x2_sampling started re-failing for 9.4 when I switched to libjpeg-turbo, and actually eps=1.2 is fine for test_get_child_images!

Sorry for the mix-up!

Perhaps it would be possible to widen the skip of test_strip_ycbcr_jpeg_2x2_sampling to take into account newer libjpeg_turbo? Currently it only skips 2.0, but we're on 2.1 (and ABI-wise, we're only pinning to 2, without the minor version).

Copy link

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

In other words, this PR is still necessary (or at least useful for us), but should be good enough with the following:

@@ -447,7 +447,7 @@ def test_get_child_images(self):
ims = im.get_child_images()

assert len(ims) == 1
assert_image_equal_tofile(ims[0], "Tests/images/flower_thumbnail.png")
assert_image_similar_tofile(ims[0], "Tests/images/flower_thumbnail.png", 2.1)

Choose a reason for hiding this comment

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

Suggested change
assert_image_similar_tofile(ims[0], "Tests/images/flower_thumbnail.png", 2.1)
assert_image_similar_tofile(ims[0], "Tests/images/flower_thumbnail.png", 1.2)

@radarhere
Copy link
Member Author

When I test with libjpeg 9e, I also found a difference of 2.0033 - https://github.com/radarhere/pillow-wheels/actions/runs/3821343292/jobs/6500396156

If we're going to change this to improve support, might as well change it to support more libjpeg environments in general.

@radarhere
Copy link
Member Author

Perhaps it would be possible to widen the skip of test_strip_ycbcr_jpeg_2x2_sampling to take into account newer libjpeg_turbo? Currently it only skips 2.0, but we're on 2.1 (and ABI-wise, we're only pinning to 2, without the minor version).

Our GitHub Actions uses libjpeg turbo 2.1 and passes - https://github.com/python-pillow/Pillow/actions/runs/3819974382/jobs/6497949271#step:8:39

If you'd like that to be investigated further, please open a new issue.

@h-vetinari
Copy link

h-vetinari commented Jan 2, 2023

Yeah, I should not have responded so quickly (but wanted to avoid an inadvertent merge in that moment).

The 2.1 remains necessary on osx, where test_strip_ycbcr_jpeg_2x2_sampling does not fail on osx, and so I had actually read the right results.

Sorry for the back and forth 😑

@h-vetinari
Copy link

Our GitHub Actions uses libjpeg turbo 2.1 and passes - https://github.com/python-pillow/Pillow/actions/runs/3819974382/jobs/6497949271#step:8:39

If you'd like that to be investigated further, please open a new issue.

Fair enough. Since it's just a minor accuracy violation I'm going to skip that test for our builds.

@hugovk hugovk merged commit d2e77de into python-pillow:main Feb 12, 2023
@radarhere radarhere deleted the child_images branch February 12, 2023 10:48
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