Next topic

[sorlok_reaves]

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?

[Josh @ Dreamland]

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!

[sorlok_reaves]

The current image_load does something like this:

Code Select Expand

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.

[TheExDeus]

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.

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 Expand

     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.

[Josh @ Dreamland]

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.)

[sorlok_reaves]

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.

[sorlok_reaves]

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.

[sorlok_reaves]

The world is not ready for ENIGMA-powered GIFs!

[Darkstar2]

The world is not ready for ENIGMA-powered GIFs!

What do you mean ? Is it NOW ready or NOT ready ? Was that a typo ?

[Josh @ Dreamland]

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.

[sorlok_reaves]

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.

[sorlok_reaves]

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