search

conda-forge / pillow-feedstock

A conda-smithy repository for pillow.
BSD 3-Clause "New" or "Revised" License
2 stars 30 forks source link

pillow 9.3.0 with libjpeg-turbo #128

Closed bollwyvl closed 1 year ago

bollwyvl commented 1 year ago

Checklist

Closes #126

conda-forge-linter commented 1 year ago

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.

h-vetinari commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Pillow 9.4.0 included https://github.com/python-pillow/Pillow/pull/6767 to fix https://github.com/python-pillow/Pillow/issues/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 commented 1 year ago

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 https://github.com/python-pillow/Pillow/pull/6767 would help us fix 9.3 after all.

radarhere commented 1 year ago

Sure, thanks. I've created https://github.com/python-pillow/Pillow/pull/6853

hmaarrfk commented 1 year ago

Please see discussion https://github.com/conda-forge/conda-forge.github.io/issues/673

jakirkham commented 1 year ago

There's now a migration PR ( https://github.com/conda-forge/pillow-feedstock/pull/137 ) adding libjpeg-turbo. Are we ok going with that? Is there anything else needed from here?

bollwyvl commented 1 year ago

This hasn't caused me any more issues... Must confess I was a bit out off my depth anyway. I'm just on my phone right now, though, so feel free to close it out!

h-vetinari commented 1 year ago

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!).

h-vetinari commented 1 year ago

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

jakirkham commented 1 year ago

Was trying to answer my own question

Is there anything else needed from here?

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