Pages: 1
  Print  
Author Topic: Animated GIFs  (Read 2728 times)
Offline (Unknown gender) sorlok_reaves
Posted on: December 13, 2014, 10:00:25 PM
Contributor
Joined: Dec 2013
Posts: 261

View Profile


So I've written a GIF parser from scratch. It loads each GIF image into its own sub-image, just like GM does. This can be used with image_load(), which would otherwise assume it was a PNG and crash.

I need to do a bit of cleanup before submitting a pull request (it's super slow right now), but I wanted some feedback first. Is this something you guys want? It's only 500 lines of code, so it shouldn't clutter much.

Thoughts?
Logged
Offline (Male) Josh @ Dreamland
Reply #1 Posted on: December 13, 2014, 10:26:37 PM

Prince of all Goldfish
Developer
Location: Pittsburgh, PA, USA
Joined: Feb 2008
Posts: 2956

View Profile Email
First thought is that the image_load function shouldn't crash, no matter what's in the file. Super slow isn't ideal, yes, but (a) it's better than nothing, and (b) it's an image loader; it shouldn't be called that frequently. Third thought: how did you handle the many GIF compositing methods? Or does it just not support animation?

My other thoughts are of decreasing relevance. Looks good! Thanks!
Logged
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble
"I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
Offline (Unknown gender) sorlok_reaves
Reply #2 Posted on: December 14, 2014, 01:38:43 AM
Contributor
Joined: Dec 2013
Posts: 261

View Profile
The current image_load does something like this:
Code: [Select]
if (format.compare(".png") == 0) {
return image_load_png(filename, width, height, fullwidth, fullheight, flipped);
} else if (format.compare(".bmp") == 0) {
return image_load_bmp(filename, width, height, fullwidth, fullheight, flipped);
} else {
return image_load_bmp(filename, width, height, fullwidth, fullheight, flipped);
}

...so yeah, it's definitely going to react badly to unknown image formats, one way or another.

Regarding your other points:

  • I'm going to be speeding up the implementation; there's lots of easy optimizations I can make.
  • I haven't added in the GIF graphics extensions, but they're actually super-easy. Regarding animation, GM treats GIFs as sub-images in sprites, so there's no timing information. We could probably hack that in, if desired.
Logged
Offline (Unknown gender) TheExDeus
Reply #3 Posted on: December 14, 2014, 07:53:34 AM

Developer
Joined: Apr 2008
Posts: 1872

View Profile
Quote
Regarding animation, GM treats GIFs as sub-images in sprites, so there's no timing information. We could probably hack that in, if desired.
I think what Josh meant is that GIF's support MANY options, like interlacing, uncompressed gifs, blocks and so on, so what of those options do your loader support? I personally don't care about those and if it loads a regular run of the mill gif, then I'm fine with that.

Quote
The current image_load does something like this:
I know that previously we didn't check for errors, even if the image_load_ functions returned them. But now it seems we do. The image_load returns an error when it couldn't load the file. Right now we call image_load_png when image_load_bmp fails, so this won't work if we have many formats as we will have to daisy chain those functions. Maybe a for cycle with all the formats would be better. image_load_png on the other hand returns NULL on fail. Then image_load() actually returns the NULL back even further into places like sprite_add_to_index() were we check for error:
Code: [Select]
     unsigned char *pxdata = image_load(filename, &width, &height, &fullwidth, &fullheight, false);
        if (pxdata == NULL) { printf("ERROR - Failed to append sprite to index!\n"); return; }
Not sure where we crash. Will have to investigate.
Logged
Offline (Male) Josh @ Dreamland
Reply #4 Posted on: December 14, 2014, 11:11:07 AM

Prince of all Goldfish
Developer
Location: Pittsburgh, PA, USA
Joined: Feb 2008
Posts: 2956

View Profile Email
I wasn't talking about timing animation, but rather about animation compression. This is done by compositing frames, and leaving large regions between frames transparent. It gave us some trouble when we wrote the APNG importer in LGM. I'm not sure how bad GIF is about it, but APNG supports some fifty million methods. (It's not actually that bad; it just caught us by surprise and was not a welcome work task.)
Logged
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble
"I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
Offline (Unknown gender) sorlok_reaves
Reply #5 Posted on: December 14, 2014, 11:50:59 AM
Contributor
Joined: Dec 2013
Posts: 261

View Profile
I'm not sure how bad GIF is about it, but APNG supports some fifty million methods. (It's not actually that bad; it just caught us by surprise and was not a welcome work task.)

GIF supports 4 methods: "clear", "previous image", "previous and restore", and another one I can't recall. They're all simple, and shouldn't be a problem.

I know that previously we didn't check for errors, even if the image_load_ functions returned them. But now it seems we do.

I'll have a look and see what crashes; it might be a divide-by-zero, actually (since the convention when loading GIFs is to set the subimage_count to 0). Will get back to you on this.
Logged
Offline (Unknown gender) sorlok_reaves
Reply #6 Posted on: December 15, 2014, 10:40:43 PM
Contributor
Joined: Dec 2013
Posts: 261

View Profile
Got it to parse a lot faster: https://github.com/sorlok/enigma-dev/blob/bd8342b92630d241367579e2b7932570d3ca3597/ENIGMAsystem/SHELL/Universal_System/gif_format.cpp

I'm currently running it through a variety of animated GIFs to try to find cases where it crashes. Once that's done, I'll make the pull request.
Logged
Offline (Unknown gender) sorlok_reaves
Reply #7 Posted on: December 17, 2014, 07:09:06 PM
Contributor
Joined: Dec 2013
Posts: 261

View Profile
The world is not ready for ENIGMA-powered GIFs!

Logged
Offline (Unknown gender) Darkstar2
Reply #8 Posted on: December 17, 2014, 10:51:35 PM
Member
Joined: Jan 2014
Posts: 1244

View Profile Email
The world is not ready for ENIGMA-powered GIFs!



What do you mean ? Is it NOW ready or NOT ready ? Was that a typo ? :D
Logged
Offline (Male) Josh @ Dreamland
Reply #9 Posted on: December 17, 2014, 11:55:54 PM

Prince of all Goldfish
Developer
Location: Pittsburgh, PA, USA
Joined: Feb 2008
Posts: 2956

View Profile Email
Left you a couple comments in the interest of efficiency, but didn't catch the segfault hazards. I'll give it another look tomorrow; I'm pretty beat, at the moment.
Logged
"That is the single most cryptic piece of code I have ever seen." -Master PobbleWobble
"I disapprove of what you say, but I will defend to the death your right to say it." -Evelyn Beatrice Hall, Friends of Voltaire
Offline (Unknown gender) sorlok_reaves
Reply #10 Posted on: December 18, 2014, 12:33:12 AM
Contributor
Joined: Dec 2013
Posts: 261

View Profile
Thanks, I'll add in the changes you mentioned. You might want to hold off on the segfault review; I've done some local changes that affect how memory is accessed.
Logged
Offline (Unknown gender) sorlok_reaves
Reply #11 Posted on: December 18, 2014, 09:41:25 PM
Contributor
Joined: Dec 2013
Posts: 261

View Profile
In case you were wondering, An Untitled Story uses animated GIFs for a card game. Works perfectly now!

Logged
Pages: 1
  Print