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

emoji-upload: Fix transparency issues on GIF emoji upload. #16456

Closed

Conversation

codypiersall
Copy link

This preserves the alpha layer on GIF images that need to be resized
before being uploaded. Two important changes occur here:

  1. The new frame is a copy of the original image, which preserves the
    GIF info.
  2. The disposal method of the original GIF is preserved. This
    essentially determines what state each frame of the GIF starts from
    when it is drawn; see PIL's docs:
    https://pillow.readthedocs.io/en/stable/handbook/image-file-formats.html#saving
    for more info.

@codypiersall
Copy link
Author

codypiersall commented Oct 2, 2020

This fixes #16370 (in theory)

Testing Plan:
Testing was not robust, and I would appreciate people to review the change. I uploaded the party parrot emoji with and without this change, and the version with the change worked perfectly, while without my change issue #16370 was a problem.

The change makes sense to me. The original code was throwing away a bunch of metadata about the original GIF image, but now all the metadata is saved (because an im.copy() is done now) and, critically it turns out, the disposal method was preserved.

GIFs or Screenshots:

Before:
bad

After:
good

Original:
parrot

There's also a separate resizing issue that would be nice to get sorted out.

@codypiersall
Copy link
Author

I'm going to bed now, but I'll check back as soon as I can. If this PR can't be merged hopefully it will help someone else to come up with a fix.

@codypiersall
Copy link
Author

codypiersall commented Oct 2, 2020

Oops, it turns out the transparency is still messed up, but I was looking at this with the light theme where the images looked transparent. Oops...

--edit: this is fixed now.

This preserves the alpha layer on GIF images that need to be resized
before being uploaded.  Two important changes occur here:

1. The new frame is a *copy* of the original image, which preserves the
   GIF info.
2. The disposal method of the original GIF is preserved.  This
   essentially determines what state each frame of the GIF starts from
   when it is drawn; see PIL's docs:
   https://pillow.readthedocs.io/en/stable/handbook/image-file-formats.html#saving
   for more info.
@codypiersall
Copy link
Author

Okay, I fixed the transparency problem by setting optimize=False in save.

@akshatdalton
Copy link
Member

@codypiersall I tried your changes but they do not seem to be resolving the transparency issue for the gifs mentioned in #16370 .
Following is the uploaded results :
bad_girl
bad_nice

According to me, the problem seems to be in Pillow itself as I tried the following simple code to view each frame of the gif and it showed the bad artifacts/background (same mentioned in #10351 ):

from PIL import Image

def simpleCollage(frames, num_images_width : int = 5, num_images_height : int = 6):
    width, height = frames.size
    compilation = Image.new('RGBA', size=(width * num_images_width, height * num_images_height))
    for i in range(frames.n_frames):
        frames.seek(i)
        the_frame = frames.convert('RGBA')
        compilation.paste(the_frame, box=(width * int(i % num_images_width), height * int(i / num_images_width)))
        if i == (num_images_width * num_images_height):
            break
    compilation.show()

def extract_frames(frames):
    for i in range(frames.n_frames):
        frames.seek(i)
        frames.show()      # showing bad artifacts

im = Image.open('girl.gif')
simpleCollage(im)       # view each frame in collage
# uncomment the following to run it
# extract_frames(im)      # just simply view all the frames

Result :
gir_collagel

Many such similar issues can be seen here.

Probably the best fix is using Wand but I think cause of dependency issue(as mentioned here in the discussion) it will not be used.

@codypiersall
Copy link
Author

Thanks for testing this out!

I'm not sure if it's something PIL can't do or if the incantation is just more esoteric... I'm worried that I may need to get a deeper understanding of the GIF image format. I've poked around at the PIL source code for the GifImageFile class and it was... hard to follow 😅

@timabbott
Copy link
Sponsor Member

Thanks for working on this @codypiersall! Do I understand correctly that this PR as written does fix some of the GIFs that were failing? If so, it might be reasonable to merge this (but not close the issue), perhaps after a bit more investigation.

@codypiersall
Copy link
Author

codypiersall commented Oct 3, 2020

@timabbott It does fix some (I was initially testing the party parrot emojis, and they worked) but not all!

I haven't found any regressions, but still haven't tried broadly very many.

I am convinced, like @akshatdalton, that there is a problem in Pillow. The simplest code that demonstrates that the problem is PIL is here:

import io

from PIL import Image
import requests

data = io.BytesIO(requests.get('https://camo.githubusercontent.com/d300a5c35cbbb53e1ef09be18de71f9d2f0bb440/68747470733a2f2f6d65646961322e67697068792e636f6d2f6d656469612f6c314a394c55704751345443747a4f32342f67697068792e676966').content)


im = Image.open(data)
print(im)
for frame in range(im.n_frames):
    im.seek(frame)
    im.show()
    input('press Enter to continue')

^ this code shows every frame of the gif, and it looks horrible, without doing any processing at all.

@timabbott
Copy link
Sponsor Member

Merged as 5dab6e9, huge thanks for your work on this @codypiersall!

Can you open an issue with PIL upstream with that test case and asking for advice on how to further investigate? It sounds like the remaining problem here is best fixed upstream, but we can make that outcome much more likely by providing a good bug report.

@codypiersall
Copy link
Author

codypiersall commented Oct 12, 2020

Thanks @timabbott!

Turns out there's already a PIL bug, which may be the same bug as here: python-pillow/Pillow#634 python-pillow/Pillow#1525

@timabbott
Copy link
Sponsor Member

Hmm, well, if you think that's the same bug, maybe we should post on the thread for it with our findings?

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

3 participants