Animated, transparent GIFs not calculating diff correctly

[Artheau]

function _write_multiple_frames in GifImagePlugin.py has an error on line 445 (line 445 in present repo, line 443 in released version 5.4.1).
Specifically, when the disposal_method is set to '2', it should calculate the diff against the background color, but currently it is calculating the diff against the previous frame
previous = im_frames[-1],
and this produces substantial artifacts in the rendering whenever there are still portions of the image in adjacent frames.

[dhsdshdhk]

Can you make a pull request to fix that?

[sircinnamon]

--- a/src/PIL/GifImagePlugin.py
+++ b/src/PIL/GifImagePlugin.py
@@ -443,7 +443,10 @@ def _write_multiple_frames(im, fp, palette):
             if im_frames:
                 # delta frame
                 previous = im_frames[-1]
-                if _get_palette_bytes(im_frame) == \
+                if "disposal" in encoderinfo and encoderinfo["disposal"] == 2:
+                    # If diposing whole frame, treat full new frame as delta
+                    delta = im_frame
+                elif _get_palette_bytes(im_frame) == \
                    _get_palette_bytes(previous['im']):
                     delta = ImageChops.subtract_modulo(im_frame,
                                                        previous['im'])

Tried this out in a fork, but the travisCI build failed on the test_save_dispose test, it seems like it's causing it to drop a frame somewhere, although I honestly cannot tell why. Open to ideas.

[Artheau]

I haven't dev'd on this project before, so please save me from myself, but I think the following snippet works. I didn't fork and pull request because the code needs a little love. It, for instance, creates a new background image on every iteration of the loop. That should probably be moved out of the loop, but then it would not have access to im_frame.size. Also, the new image should probably be created with the same palette, but again, if it is created outside of the loop then it is not clear how this would be accomplished.

Importantly, this code does not just use the whole frame as the delta. It does continue to try to optimize the frames as much as possible by comparing against the background color.

In any case, I think this code works, but please help me optimize the number of calls to Image.new:

            if im_frames:
                # delta frame
                previous = im_frames[-1]
                if ("disposal" in im.encoderinfo and im.encoderinfo["disposal"] == 2):
                    base_image = Image.new("P", im_frame.size, 0)
                else:
                    base_image = previous["im"]

                if _get_palette_bytes(im_frame) == \
                   _get_palette_bytes(base_image):
                    delta = ImageChops.subtract_modulo(im_frame,
                                                       base_image)
                else:
                    delta = ImageChops.subtract_modulo(
                        im_frame.convert('RGB'), base_image.convert('RGB'))
                bbox = delta.getbbox()
                if not bbox and not ("disposal" in im.encoderinfo and im.encoderinfo["disposal"] == 2):
                    # This frame is identical to the previous frame
                    if duration:
                        previous['encoderinfo']['duration'] += \
                            encoderinfo['duration']
                    continue

[radarhere]

Is there sample code that demonstrates this problem?

[burnoo]

@radarhere

import requests
from PIL import Image
response = requests.get('https://66.media.tumblr.com/tumblr_lzkp40b0Sl1qhwcy0.gif')
im = Image.open(BytesIO(response.content))
im.save('1.png', 'PNG')
im.seek(1)
im.load()
im.save('2.png', 'PNG')

The 2.png has second frame pasted onto first.
Expected result is second frame pasted into background (transparent).

[sircinnamon]

@Artheau With further testing, this introduces a new problem - each frame generates a diff compared to a colour 0 image - but each frame calculates its own palette and so colour 0 is not the same (nor are and colours necessarily.

I've tested and it seems like if every colour that appears in any frame appears in the first frame, it all works great. Trying to work out a solution where a palette that includes every colour from every frame can be generated and used for every frame.

[Artheau]

Yeah, it seems like that could come up for some cases.

So, the code seems to already compare the palettes: if _get_palette_bytes(im_frame) == _get_palette_bytes(base_image):, and in the best case, the palettes are the same, so the zero palette comparison will work.

I think an elegant solution would probably try to intelligently compare the palettes and find out what the palette position is in the new palette, defaulting to using the whole image as the diff if the color is not in the new palette...somehow this seems like overkill, because the existing code doesn't do that at all.

If we riff off of the existing code, the corresponding else clause handles the case where the palettes do not exactly match, which includes the case you are concerned about. In such a case, the existing code resolves the paletted images to RGB equivalents. delta = ImageChops.subtract_modulo(im_frame.convert('RGB'), base_image.convert('RGB')) So...it makes sense that in such cases, we could just compare against some kind of Image.new("RGB", im_frame.size, <background_color>). Perhaps there is there a convenient way to extract the RGB color corresponding to palette position 0 of the first image, or somehow give the base_image enough information to be able to natively convert correctly to RGB?

[sircinnamon]

Unfortunately, even if we use the whole frame as the diff, if it doesn't have the same palette as the first frame the colour mapping will be wrong. We will need to enforce the same palette onto every frame I think.

[Artheau]

What does the code do presently if images are presented with different palettes but disposal mode 2 is not activated? Does it work correctly? If so, I think we should just create an RGB background image to compare against instead of comparing against the paletted version.

[sircinnamon]

        im_list = [
            Image.new('RGB', (100, 100), '#fff'),
            Image.new('RGB', (100, 100), '#999'),
            Image.new('RGB', (100, 100), '#000'),
        ]
        for img in im_list:
            d = ImageDraw.Draw(img)
            d.ellipse([(0,0),(100,100)], fill='#f00')
            # d.ellipse([(0,0),(1,1)], fill='#fff')
            # d.ellipse([(2,2),(3,3)], fill='#999')
            # d.ellipse([(4,4),(5,5)], fill='#000')
        im_list[0].save("test0.png")
        im_list[1].save("test1.png")
        im_list[2].save("test2.png")

        # check per frame disposal
        im_list[0].save(
            out,
            save_all=True,
            append_images=im_list[1:],
            disposal=0
            )

This is my test case - a red circle over a background changing white to gray to black. Right now, regardless of disposal mode, the output frames are: red circle on white (correct), red circle on white (incorrect) and white circle on red (very incorrect). My assumption is that the colours of the first frame are being mapped to the other frames (though that could be incorrect)

[sircinnamon]

Okay, I've made a discovery which might totally shift how I do this:

Each frame is capable of storing it's own palette locally, so we don't have to worry about all frames sharing a palette

In order to include that local colour table header, the frame info must include include_color_table:True and have a non-zero size (as determined by _get_color_table_size in GifImagePlugin.py)

The formula for the colour table size
is int(math.ceil(math.log(len(palette_bytes)//3, 2)))-1 which I can't fully understand - the divide by 3 makes sense because each colour is stored as 3 bytes, but the log, ceil and -1 are confusing to me. I checked and if a frame has only 2 colours, this get colour table size returns a 0. I tried upping the frames to have 3 colours and suddenly the test worked.

[sircinnamon]

@radarhere It looks like the color_table_size calculation was written by you, I'm wondering if you have any insight? It looks like a frame with the colours b'\x00\x00\x00\xff\x00\x00' comes back as a 0 for color_table_size but b'\x00\x00\x00\xff\x00\x00\x00\x00\xff'comes back fine. I mean I'm clearly missing something because it works for every other image except this case.

Where does the -1 come from? Would it be appropriate to just force a >3 len palette for disposal=2 frames?

[radarhere]

That calculation was actually added in a466b3e, not written by me. That said, https://www.w3.org/Graphics/GIF/spec-gif89a.txt states that

the value in this field is used to calculate the number of bytes contained in the Global Color Table. To determine that actual size of the color table, raise 2 to [the value of the field + 1]

So int(math.ceil(math.log(..., 2)))-1 is just the inverse of that.

[sircinnamon]

That's on me for not reading the history close enough, I just saw the last modification and assumed, I apologize for that!

Okay so that confirms that the color table basically has a minimum palette size of 3 colors (or two if we remove the statement that considers 0 invalid) and that padding out the palette if it's too small is probably the way to go! Thank you!

[radarhere]

The sample code provided by @burnoo is not actually solved by #3708 - that's okay though, because it is solved by #3434. I'm going to suggest that any discussion of that problem is moved to #3153, which looks like the same issue.

[radarhere]

Here are some thoughts.

The disposal method determines how a GIF frame is removed. A value of 2 means that in the next frame after the frame when it is added, it should be replaced by the background colour. See one of Pillow's test images to demonstrate -

When writing a GIF image, because GIFs are effectively made up of different images that are pasted on top of each other as they move along, each subsequent image can be made smaller by cropping it to only include the section of the image that has changed compared to the previous image. This cropped image is also referred to in the code as a delta, and referred to in this issue as a diff.

So the delta controls how an image is added, and disposal controls how an image is removed, meaning that the two are not related when writing out a file. Without code to demonstrate something wrong, I don't see a problem here. Yes, there is currently a problem within Pillow where the palettes are read incorrectly on subsequent frames, but that's a reading bug, not a writing bug.

Feel free to disagree, or to add code showing a problem.

[sircinnamon]

Disposal and Delta are inherently linked, because you can't layer a delta on top of a frame that's been discarded underneath - you need to calculate the delta in a totally different way (compared to the background of the gif)

The problem with the current setup is the delta is calculated based on the frame that is disposed so anything that didn't change is erased. Here is a clear example (which I will turn into a unit test)

out = 'temp.gif'

 # 4 frames: red/blue, red/red, blue/blue, red/blue
circles = [
    ((255, 0, 0, 255),(0, 0, 255, 255)),
    ((255, 0, 0, 255),(255, 0, 0, 255)),
    ((0, 0, 255, 255),(0, 0, 255, 255)),
    ((255, 0, 0, 255),(0, 0, 255, 255))
]

im_list = []
for i in range(len(circles)):
    img = Image.new('RGBA', (100, 100), (255,255,255,0))

     # Red circle in center of each frame
    d = ImageDraw.Draw(img)
    d.ellipse([(0, 30), (40, 70)], fill=circles[i][0])
    d.ellipse([(60, 30), (100, 70)], fill=circles[i][1])

    im_list.append(img)

im_list[0].save(
    out,
    save_all=True,
    append_images=im_list[1:],
    disposal=2,
    transparency=0
)

In any frame where a circle is supposed to be the same colour as the frame before, it doesn't show up at all

[Artheau]

For me this has issue initially came about by implementing animation with a transparent background. If the body stays still and only a small portion of the image moves, the present code will often remove the entire body. Example:

from PIL import Image

for mode in [1,2]:
	images = [Image.open(f"test{i}.png") for i in range(4)]
	images[0].save(f"disposal_test{mode}.gif", 'GIF', save_all=True, append_images=images[1:], duration=500, transparency=0, disposal=mode, loop=0)

Results:
Disposal method 1:

Disposal method 2: