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

Fix "restore to previous" #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tduva
Copy link

@tduva tduva commented Mar 1, 2019

The disposal method "restore to previous" should restore to what was there before the graphic is drawn, so to the image data after the disposal of the previous frame, not to the previous frame. This disposal method isn't used very often, but I've encountered a few GIFs where it is an issue. A few examples are on this page.

The GIF Spec says:

3 - Restore to previous. The decoder is required to
restore the area overwritten by the graphic with
what was there prior to rendering the graphic.

Which I didn't quite know how to interpret exactly (and there do seem to be different interpretations on some websites that explain how it's supposed to work), but based on the GIFs I encountered and how modern browsers handle them, this fix should be correct.

Example

bunny

Following are screenshots of the individual frames of this GIF (top row the finished frame, bottom row what each frame adds). In this case, the GIF uses "restore to previous" to fully clear each frame (which works because each frame uses it, so it always restores to the empty canvas present before drawing the first frame's graphic).

Original:
gifdecoder_unfixed

Fixed:
gifdecoder_fixed


This change is my own work and while I'm not convinced I can legally disclaim or transfer copyright, you can see this change as being under CC0, so effectively Public Domain.

The disposal method "restore to previous" should restore to what was
there before the graphics is drawn, not the previous finished frame.

Based on tests this fixed behaviour also seems to be consistent with
what modern browsers do.
@rtyley
Copy link
Owner

rtyley commented Mar 1, 2019

Hi @tduva thanks for this contribution - it looks great, thanks for taking the time to do this! Could you add a test case to TestGifDecoder so we can have a unit test for this feature?

Note that depending on the version of Java you're using, you may need to rebase on top of the latest master commit (2a3b078) to get the current tests to pass.

@tduva
Copy link
Author

tduva commented Mar 24, 2019

I've been busy, but I'll get around to adding a test eventually..

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.

2 participants