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

Fill only the area that was modified by the previous GIF frame #1093

Merged
merged 1 commit into from
Mar 25, 2016

Conversation

TWiStErRob
Copy link
Collaborator

Description

As seen in #1059:
flickering GIF animation

If you look closely you can see there are multiple different flickers:

  • one where the frame is partially shown
  • one where a single frame is almost fully transparent

This is how it should look like:
original image

This change achieves that by clearing only part of the frame when need to dispose to background.

Motivation and Context

I sent the above image through http://ezgif.com/split ("Ignore optimizations") to see what how they look:
image

As you can see there's an actual half-frame and there are many almost-empty frames, most importantly the one where there's a little pink on the left. These two frames cause the trouble. Their dispose is set to "background" which triggers a code block that fills the entire frame with bgColor. The problem is that the frame doesn't contain a full-sized image, just a partial one. This is also confirmed in debug (closed frames look identical to 0th frame):
image

I checked the GIF spec and it says:

iv) Disposal Method - Indicates the way in which the graphic is to be treated after being displayed.
2 - Restore to background color. The area used by the graphic must be restored to the background color. -- GIF89a, emphasis mine

This means that when preparing the frame transition from 2 to 3, 2 wants it's partial area to be cleared as defined by the (ix,iy iw,ih) rect. I took a shot at implementing that, I went through these iterations to arrive at the code in the PR.

  1. simple iteration with cached end values

    for (int y = previousFrame.iy, yEnd = y + previousFrame.ih; y < yEnd; y++) {
        for (int x = previousFrame.ix, xEnd = x + previousFrame.iw; x < xEnd; x++) {
            dest[y * width + x] = c;
        }
    }
  2. pre-calculate values and name them

    int top = previousFrame.iy;
    int bottom = previousFrame.iy + previousFrame.ih;
    int left = previousFrame.ix;
    int right = previousFrame.ix + previousFrame.iw;
    for (int y = top; y < bottom; y++) {
        for (int x = left; x < right; x++) {
            dest[y * width + x] = c;
        }
    }
  3. lift multiplication up

    int top = previousFrame.iy;
    int bottom = previousFrame.iy + previousFrame.ih;
    for (int y = top; y < bottom; y++) {
        int left = y * width + previousFrame.ix;
        int right = y * width + previousFrame.ix + previousFrame.iw;
        for (int p = left; p < right; p++) {
            dest[p] = c;
        }
    }
  4. lift multiplication up again and rename variables

    int topLeft = previousFrame.iy * width + previousFrame.ix;
    int bottomLeft = topLeft + previousFrame.ih * width;
    for (int left = topLeft; left < bottomLeft; left += width) {
        int right = left + previousFrame.iw;
        for (int pointer = left; pointer < right; pointer++) {
            dest[pointer] = c;
        }
    }

Not sure which one is best, I think the final version is readable enough and all multiplications are lifted outside, so it should be efficient as well.

@TWiStErRob
Copy link
Collaborator Author

/cc @kojilin

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.158% when pulling e15172e on TWiStErRob:gif_dispose into e68db00 on bumptech:3.0.

@sjudd
Copy link
Collaborator

sjudd commented Mar 25, 2016

LGTM, awesome work!

@sjudd sjudd merged commit 258cc10 into bumptech:3.0 Mar 25, 2016
@TWiStErRob TWiStErRob deleted the gif_dispose branch March 25, 2016 23:51
TWiStErRob added a commit to TWiStErRob/glide that referenced this pull request Jul 10, 2016
…y the previous GIF frame on dispose background
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.

3 participants