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 v9.0.0 breaks test suite #719

Closed
FirefoxMetzger opened this issue Jan 3, 2022 · 2 comments · Fixed by #724
Closed

Pillow v9.0.0 breaks test suite #719

FirefoxMetzger opened this issue Jan 3, 2022 · 2 comments · Fixed by #724

Comments

@FirefoxMetzger
Copy link
Contributor

FirefoxMetzger commented Jan 3, 2022

Pillow v9 introduced a change that broke our GIF reading test in the legacy pillow plugin:

def test_animated_gif():
# Read newton's cradle
ims = imageio.mimread("imageio:newtonscradle.gif")
assert len(ims) == 36
for im in ims:
assert im.shape == (150, 200, 4)
assert im.min() > 0
assert im.max() <= 255
# Get images
im = get_ref_im(4, 0, 0)
ims = []
for i in range(10):
im = im.copy()
im[:, -5:, 0] = i * 20
ims.append(im)
# Store - animated GIF always poops out RGB
for isfloat in (False, True):
for colors in (3, 4):
ims1 = ims[:]
if isfloat:
ims1 = [x.astype(np.float32) / 256 for x in ims1]
ims1 = [x[:, :, :colors] for x in ims1]
fname = fnamebase + ".animated.%i.gif" % colors
imageio.mimsave(fname, ims1, duration=0.2)
# Retrieve
print("fooo", fname, isfloat, colors)
ims2 = imageio.mimread(fname)
ims1 = [x[:, :, :3] for x in ims] # fresh ref
ims2 = [x[:, :, :3] for x in ims2] # discart alpha
for im1, im2 in zip(ims1, ims2):
assert_close(im1, im2, 1.1)
# We can also store grayscale
fname = fnamebase + ".animated.%i.gif" % 1
imageio.mimsave(fname, [x[:, :, 0] for x in ims], duration=0.2)
imageio.mimsave(fname, [x[:, :, :1] for x in ims], duration=0.2)
# Irragular duration. You probably want to check this manually (I did)
duration = [0.1 for i in ims]
for i in [2, 5, 7]:
duration[i] = 0.5
imageio.mimsave(fnamebase + ".animated_irr.gif", ims, duration=duration)
# Other parameters
imageio.mimsave(fnamebase + ".animated.loop2.gif", ims, loop=2, fps=20)
R = imageio.read(fnamebase + ".animated.loop2.gif")
W = imageio.save(fnamebase + ".animated.palettes100.gif", palettesize=100)
assert W._writer.opt_palette_size == 128
# Fail
assert raises(IndexError, R.get_meta_data, -1)
assert raises(ValueError, imageio.mimsave, fname, ims, palettesize=300)
assert raises(ValueError, imageio.mimsave, fname, ims, quantizer="foo")
assert raises(ValueError, imageio.mimsave, fname, ims, duration="foo")
# Add one duplicate image to ims to touch subractangle with not change
ims.append(ims[-1])
# Test subrectangles
imageio.mimsave(fnamebase + ".subno.gif", ims, subrectangles=False)
imageio.mimsave(fnamebase + ".subyes.gif", ims, subrectangles=True)
s1 = os.stat(fnamebase + ".subno.gif").st_size
s2 = os.stat(fnamebase + ".subyes.gif").st_size
assert s2 < s1
# Meta (dummy, because always {})
imageio.mimsave(fname, [x[:, :, 0] for x in ims], duration=0.2)
assert isinstance(imageio.read(fname).get_meta_data(), dict)

It fails with:

============================= test session starts =============================
platform win32 -- Python 3.8.8, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: c:\Users\Sebastian\Documents\Coding-Projects\imageio
plugins: cov-2.11.1, localserver-0.5.0
collected 1 item

tests\test_pillow_legacy.py F                                            [100%]

================================== FAILURES ===================================
______________________________ test_animated_gif ______________________________

    def test_animated_gif():
    
        # Read newton's cradle
        ims = imageio.mimread("imageio:newtonscradle.gif")
        assert len(ims) == 36
        for im in ims:
>           assert im.shape == (150, 200, 4)
E           assert (150, 200, 3) == (150, 200, 4)
E             At index 2 diff: 3 != 4
E             Use -v to get the full diff

tests\test_pillow_legacy.py:270: AssertionError
- generated xml file: C:\Users\SEBAST~1\AppData\Local\Temp\tmp-22744GqVwFx95evBK.xml -
=========================== short test summary info ===========================
FAILED tests/test_pillow_legacy.py::test_animated_gif - assert (150, 200, 3) ...
============================== 1 failed in 0.46s ==============================

The new (v3) pillow plugin is unaffected.

At the time of this writing, I don't know what causes this problem; however, if I find the time this week, I will check how to resolve this.

Edit: Looking at the AssertionError, it appears that this is because pillow now returns a RGB image instead of returning a RGBA image.

@FirefoxMetzger
Copy link
Contributor Author

I did some digging. The change in pillow that introduced this problem is mentioned in their changelog here:

https://pillow.readthedocs.io/en/stable/releasenotes/9.0.0.html#convert-subsequent-gif-frames-to-rgb-or-rgba

I opened python-pillow/Pillow#5929 to ask if this can be made more consistent and the way we should interact with this here depends a bit on the result of the discussion there.

The gist of the issue is that the first frame of a GIF now has (pil-)mode P while all subsequent ones have mode RGB/RGBA. Which causes problems for us, because legacy pillow converts mode P into mode RGBA while leaving mode RGB untouched. This creates a problem for paletted RGB GIFs, resulting in the above exception.

We could simply skip pillow v9 for now, but my preferred approach would be to fix it correctly/properly because there are likely other worthwhile changes in pillow v9 that we can benefit from, and this seems like a small problem in comparison.

@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Jan 5, 2022

I did some more digging, and this issue doesn't seem to resolve easily on our end. For GIF, we default to returning RGBA with the v2 pillow plugin, even if the GIF uses an RGB palette. This is problematic with pillow9, because each frame beyond the first will be returned as an RGB image (for RGB pallet GIFs) and our reader doesn't keep track of previously read frames. To us, each frame beyond the first looks like any other RGB image (which we don't promote to RGBA; I tried changing that, but that breaks a lot of other tests and isn't backward compatible).

A proper solution would be to make this consistent and to rewrite parts of the v2 pillow plugin. However, the v3 pillow plugin already does that, so this would be redundant effort that will become obsolete quite quickly.

@almarklein What is your preferred way forward here? We could: (1) mark the test as xfail and see if this causes trouble for any user, (2) limit ourselves to pillow<9.0.0 until we fully depreciate the v2 pillow plugin.

Edit: Nvm the above. I found a way to solve this that only involves minimal changes on our end :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant