Navigation Menu

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

Create GIF deltas from background colour of GIF frames if disposal mode is 2 #3708

Merged
merged 23 commits into from Jun 29, 2019

Conversation

sircinnamon
Copy link
Contributor

Fixes #3665
All credit to @Artheau who figured out the method, I just tidied up and made sure it passed the tests+lint

Changes proposed in this pull request:

  • Save a "background colour" image from the first frame image, and use that to create gif deltas if disposal mode is 2

@sircinnamon sircinnamon changed the title Create gif deltas from background colour of gif frames id disposal mode is 2 Create GIF deltas from background colour of GIF frames if disposal mode is 2 Mar 11, 2019
@radarhere
Copy link
Member

Are you able to add tests, so that these changes won't be reverted by future changes?

@sircinnamon
Copy link
Contributor Author

Writing a test has shown me this solution is an improvement but is not complete. There is an issue where the palette of the first frame is forced upon the following frames and so any colours that "show up" later are mismatched. Going to keep working on it - I'll post my test so anyone else can check it out and compare:

def test_dispose2_diff(self):
        out = self.tempfile('temp.gif')
        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')

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

        img = Image.open(out)
        top_left_pixels = []
        center_pixels = []

        rgb_img = img.convert('RGB')
        r, g, b = rgb_img.getpixel((1,1))
        top_left_pixels += [(r,g,b)]
        r, g, b = rgb_img.getpixel((50,50))
        center_pixels += [(r,g,b)]

        for i in range(2):
            img.seek(img.tell() + 1)
            r, g, b = rgb_img.getpixel((1,1))
            top_left_pixels += [(r,g,b)]
            r, g, b = rgb_img.getpixel((50,50))
            center_pixels += [(r,g,b)]
            for prev in top_left_pixels[:i]:
                # Check bacground changed every frame
                self.assertNotEqual((r,g,b), prev)
            for prev in center_pixels[:i]:
                # Check center remains the same every frame
                self.assertEqual((r,g,b), prev)

This creates a gif with a red circle surrounded but a changing white->grey->black corners filling out the square. then it loads the gif back in and pulls a pixel at the edge and at the center and checks the colours are expected (currently they are not).

@sircinnamon
Copy link
Contributor Author

@radarhere Added a test which very carefully checks that each frame of a gif correctly changes with disposal mode 2 and checks that the colours are right (because the palettes were screwing up).

The test showed that the previous solution was incomplete. Added a little more that forces the include_color_table flag for that disposal mode, and also (across all modes) if that flag is set, pad the table to be long enough to pass the _get_color_table_size check (less than 3 colours returns a 0 from that, my change just concats the palette with itself until its longer than 3 colours)

@radarhere
Copy link
Member

The only change required to the source code to made your tests pass is the expanding of the palette. Removing the rest of your changes don't actually cause the new tests to fail, meaning that the tests that you've added don't guard against regression.

@sircinnamon
Copy link
Contributor Author

You're right, I've renamed and slightly modified that test to represent that it's testing the palette and written a new one that tests that unchanged pixels still appear on subsequent frames

@sircinnamon
Copy link
Contributor Author

Is there anything else I can do for this PR or is it waiting on another PR? I'd love to get this merged in :)

@hugovk
Copy link
Member

hugovk commented Jun 28, 2019

@radarhere What do you think, is this good to merge once the conflicts are sorted?

@radarhere
Copy link
Member

Okay, I resolved the conflicts, pushed some commits, and I'm now happy with the changes to GifImagePlugin.

@hugovk hugovk merged commit 6459caf into python-pillow:master Jun 29, 2019
@hugovk
Copy link
Member

hugovk commented Jun 29, 2019

Thanks both!

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

Successfully merging this pull request may close these issues.

Animated, transparent GIFs not calculating diff correctly
3 participants