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 swapped pixels in GifDecoder's output #3002

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

mtopolnik
Copy link
Contributor

StandardGifDecoder uses modified public domain code to decode the GIF. Some changes were done to it that introduced a subtle bug: the initial two pixels are swapped, as well as the pixels after each CLEAR command in the LZW stream.

Comparing with the original code, the bug was introduced because the loop that pops the bytes from the stack into the output pixel array was moved. This code motion did not take into account the continue in the branch guarded by oldCode == NULL_CODE. The pixel that this branch pushes to the stack was supposed to be immediately transferred to the output, but since the stack-popping loop was moved, it happens after adding first to the output pixels, thereby swapping the two initial pixels.

The fix is simple: instead of pushing to the stack in the oldCode == NULL_CODE branch, send the pixel directly to the output pixel array.

This PR includes a fix, a small test GIF with 8 white-black-white-black pixels, and a JUnit test that fails without the fix.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@mtopolnik
Copy link
Contributor Author

I signed the CLA just now.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Apr 1, 2018
@stale
Copy link

stale bot commented Apr 8, 2018

This issue has been automatically marked as stale because it has not had activity in the last seven days. It will be closed if no further activity occurs within the next seven days. Thank you for your contributions.

@stale stale bot added the stale label Apr 8, 2018
@mtopolnik
Copy link
Contributor Author

Thank you for your attention, googlebot and stalebot.

@stale stale bot removed the stale label Apr 8, 2018
@sjudd sjudd merged commit 7fb8b12 into bumptech:master Apr 9, 2018
@sjudd
Copy link
Collaborator

sjudd commented Apr 9, 2018

Thank you! Sorry for the delay. Nice find.

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