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 incorrect dealing with offset sub-frames #18

Merged
merged 1 commit into from Jul 21, 2019

Conversation

Jimbly
Copy link
Contributor

@Jimbly Jimbly commented Mar 27, 2019

gifwrap fails to correctly decode test/fixtures/rnaples-offsets-public.gif included in its tests. I threw together a simple test that combines all frames into one image with Jimp.

Erroneous results:
out_bad_128

Expected results (after applying fix):
out_good_128

omggif's decodeAndBlitFrameRGBA blits the entire frame, not just the sub-region, so, in order to keep the GifCode API the same, we must contract the buffer, skipping all of the empty space.

An alternative solution would be to simply set info.[width/height] = reader.[width/height]; info.[x/y] = 0, which would be slightly faster in here (but cause clients to be slower, as they have to blend the whole image).

Better yet would be to fix omggif (or use a different library) to allow returning just the sub-section, however I looked into decodeAndblitFrameRGBA and it gets pretty complicated due to interlacing and stuff.

But, for a quick fix, this'll do.

@devinivy
Copy link

I can't verify the fix itself, but this solves similar problems I was having with an animated gif as well.

@Jimbly
Copy link
Contributor Author

Jimbly commented Jun 20, 2019

Yeah, without this fix, I seem to recall that any animated GIFs that animated a sub-frame (don't replace the entire frame each time) won't work.

@devinivy
Copy link

devinivy commented Jun 20, 2019

Ah! Yeah, that is consistent with what I've seen. At least the glitch looks neat :)

@jtlapp jtlapp merged commit de5ac12 into jimp-dev:master Jul 21, 2019
jtlapp added a commit that referenced this pull request Jul 21, 2019
@jtlapp
Copy link
Collaborator

jtlapp commented Jul 21, 2019

Thank you sooo much for finding and fixing this problem. For now, we'll call it expected interface protocol for Omggif. I also added a test to confirm expected behavior.

@jtlapp jtlapp mentioned this pull request Jul 21, 2019
@Jimbly
Copy link
Contributor Author

Jimbly commented Jul 21, 2019

Glad to help, thanks for merging!

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.

None yet

3 participants