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

pillow 9.3.0 with libjpeg-turbo #128

Closed
wants to merge 7 commits into from

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Nov 17, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Closes #126

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@bollwyvl bollwyvl mentioned this pull request Nov 17, 2022
3 tasks
@h-vetinari
Copy link
Member

h-vetinari commented Jan 2, 2023

So, the failing test here is known to be failing with libjpeg_turbo, and marked as such even in the source (and shows up in the logs here):

______________ TestFileLibTiff.test_strip_ycbcr_jpeg_2x2_sampling ______________

self = <Tests.test_file_libtiff.TestFileLibTiff object at 0x7f2cd2703220>

    @mark_if_feature_version(
        pytest.mark.valgrind_known_error, "libjpeg_turbo", "2.0", reason="Known Failing"
    )
    def test_strip_ycbcr_jpeg_2x2_sampling(self):
        infile = "Tests/images/tiff_strip_ycbcr_jpeg_2x2_sampling.tif"
        with Image.open(infile) as im:
>           assert_image_similar_tofile(im, "Tests/images/flower.jpg", 1.2)

It seems that skip might not be triggering because we're already on libjpeg-turob 2.1 rather than 2.0.

On top of that, it's only an accuracy failure:

>           assert epsilon >= ave_diff, (
                (msg or "")
                + f" average pixel value difference {ave_diff:.4f} > epsilon {epsilon:.4f}"
            )
E           AssertionError:  average pixel value difference 1.4928 > epsilon 1.2000

So I feel it's a non-issue (triply so...) to skip this test.

@h-vetinari
Copy link
Member

@radarhere
Maybe you could help out here. We were thinking of switching to libjpeg-turbo (a longstanding TODO we've had for this feedstock), but there's a failing test on osx test_load_blp1. It only fails on OSX, however, there it does fail severely:

E           AssertionError:  average pixel value difference 537.3142 > epsilon 1.2000

Interestingly, that same test is failing in #126 even built against regular jpeg (where it also fails against linux/windows!), however, it's passing as of 9.4 (#132; built against jpeg; however, that PR runs into another - single - failing test on all platforms: test_get_child_images)

@radarhere
Copy link
Contributor

Pillow 9.4.0 included python-pillow/Pillow#6767 to fix python-pillow/Pillow#6741. In that issue, test_load_blp1 was failing when run with libjpeg. You seem to be saying it is also failing for libjpeg-turbo, but as it's a past version of Pillow, there may not be much point in discussing that.

As for the failure with test_get_child_images, in order to pass with libjpeg, I imagine we just need to relax the check slightly, from assert_image_equal_tofile to assert_image_similar_tofile.

I don't think you've tried libjpeg-turbo with Pillow 9.4 though. Does that pass both test_load_blp1 and test_get_child_images?

@h-vetinari
Copy link
Member

Thanks for the quick feedback!

I don't think you've tried libjpeg-turbo with Pillow 9.4 though. Does that pass both test_load_blp1 and test_get_child_images?

I just tried in #32, including a patch to downgrade test_get_child_images to similarity. Didn't know what eps to take, so I took the one from test_load_blp1, but this still fails (linux/win):

E           AssertionError:  average pixel value difference 1.4928 > epsilon 1.2000

On OSX:

E           AssertionError:  average pixel value difference 2.0033 > epsilon 1.2000

Would eps=2.1 still be acceptable IYO? In the meantime, I'm also trying if backporting python-pillow/Pillow#6767 would help us fix 9.3 after all.

@radarhere
Copy link
Contributor

Sure, thanks. I've created python-pillow/Pillow#6853

@h-vetinari h-vetinari marked this pull request as ready for review January 2, 2023 09:55
@hmaarrfk hmaarrfk marked this pull request as draft February 12, 2023 00:17
@hmaarrfk
Copy link
Contributor

Please see discussion conda-forge/conda-forge.github.io#673

@jakirkham
Copy link
Member

There's now a migration PR ( #137 ) adding libjpeg-turbo. Are we ok going with that? Is there anything else needed from here?

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Mar 8, 2023 via email

@h-vetinari
Copy link
Member

Are we ok going with that? Is there anything else needed from here?

AFAIU this time it's being done properly (i.e. a conda-forge-wide migration, rather than just changing pillow), mainly driven by @hmaarrfk (thanks!).

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Looking through things here, the two notable changes appear to be addressed in other ways. There are some minor patch updates, but these likely already are included with updates. Not seeing anything else major we are missing, but please feel free to correct me if I missed something

Comment on lines +1 to +4
From f9343cc87028379fa85597475da4b0ef2e915a15 Mon Sep 17 00:00:00 2001
From: Andrew Murray <radarhere@users.noreply.github.com>
Date: Wed, 30 Nov 2022 13:49:07 +1100
Subject: [PATCH 5/5] Do not trust JPEG decoder to determine image is CMYK
Copy link
Member

Choose a reason for hiding this comment

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

This patch went in pillow version 9.4.0 with PR ( python-pillow/Pillow#6767 ). So presumably it is no longer needed as long as the latest version is building

Comment on lines -6 to -9
# Test JPEG2k
with fsspec.open("https://www.fnordware.com/j2k/relax.jp2") as f:
image = Image.open(f)
image.load()
Copy link
Member

@jakirkham jakirkham Mar 8, 2023

Choose a reason for hiding this comment

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

This was already dropped in commit ( 2e44e14 ) (as the full test suite is now run)

@h-vetinari
Copy link
Member

h-vetinari commented Mar 8, 2023

@jakirkham, no need to review here, this PR was completely obsoleted by #132 (see discussion here), but just never closed.

@jakirkham
Copy link
Member

Was trying to answer my own question

Is there anything else needed from here?

Though yeah sounds like nothing is needed. Let's close

@jakirkham jakirkham closed this Mar 8, 2023
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.

None yet

7 participants