Decode and maybe color errors with gifs from gifski

[JarvyJ]

Describe the bug
Gifs created with gifski don't seem to work for me. They do a lot of color optimization, so could be something weird there. Hopefully it's just a config thing on my end.

To Reproduce
While I am running this as a shared library loaded into another program (all on linux, not sure if the arduino code is affected), I was able to reproduce the issue using the linux example code modified to draw COOKED (great name btw) w/buffers (code below).

It claims the gif only has 4 frames, but it has more. Other gifs also fail after going through an incorrect number of frames. iError claims it's a GIF_DECODE_ERROR (1). For ones that don't have a decode error, I get some weird colors back.

#include "../src/AnimatedGIF.h"
#include "../src/gif.inl"

#include "../test_images/badgers.h"

GIFIMAGE gif;

// the decode error happens even with an empty GIFDraw
void GIFDraw(GIFDRAW *pDraw)
{
} /* GIFDraw() */

// I'm doing a simple copy out when I get weird colors
/*
void GIFDraw(GIFDRAW *pDraw)
{
    memcpy((pDraw->pUser) + ((((pDraw->iY + pDraw->y) * pDraw->iCanvasWidth) + pDraw->iX) * 3), pDraw->pPixels, pDraw->iWidth*3);
}
*/

uint8_t *pStart;

int main(int argc, const char * argv[]) {
char szTemp[256];
int rc, iFrame, w, h;

    printf("Animated GIF Linux Demo\n");
    printf("Run with no parameters to test in-memory decoding\n");
    printf("Or pass a filename on the command line\n\n");
    GIF_begin(&gif, GIF_PALETTE_RGB888);
    printf("Starting GIF decoder...\n");
    if (argc == 2) // use filename
        rc = GIF_openFile(&gif, argv[1], GIFDraw);
    else
        rc = GIF_openRAM(&gif, (uint8_t *)badgers, sizeof(badgers), GIFDraw);
    if (rc)
    {
        printf("Successfully opened GIF\n");
	w = GIF_getCanvasWidth(&gif);
	h = GIF_getCanvasHeight(&gif);
        printf("Image size: %d x %d\n", w, h);
	gif.pFrameBuffer = (uint8_t*)malloc(w * h * 3);
	pStart = &gif.pFrameBuffer[w*h];
	gif.ucDrawType = GIF_DRAW_COOKED;
	gif.pTurboBuffer = (uint8_t*)malloc(TURBO_BUFFER_SIZE + (w*h));

        iFrame = 0;
        while (GIF_playFrame(&gif, NULL, NULL))
        {
            iFrame++;
            printf("Successfully decoded frame %d\n", iFrame);
        }
        if (GIF_getComment(&gif, szTemp))
            printf("GIF Comment: \"%s\"\n", szTemp);
	printf("Error: %d\n", GIF_getLastError(&gif));
        GIF_close(&gif);
    }
    return 0;
}

Expected behavior
Ideally, it would be cool if this worked, but if not (because of something from gif.ski's end), we'll make do!

Image files
fails after 4 frames:

fails after 1 frame:

Also, the two example images from the gif.ski website have the odd colors (seem to decode just fine).

For the jazz-chromecast, my second frame looks like:

Additional context
Thanks for making this library! It's the best gif decoder that works well on lower powered hardware.

[bitbank2]

I'm able to reproduce the problem - working on it...

[bitbank2]

ok, decode error is fixed (for my test cases). Please give the newest code (not released, just 1 commit).

[JarvyJ]

All my test files seem to decode now! The colors are still odd after the first frame (for me, might just be my code).

frame 2s from the test files above:

edit:

// I'm doing a simple copy out over the old frame data when I get weird colors
void GIFDraw(GIFDRAW *pDraw)
{
    memcpy((pDraw->pUser) + ((((pDraw->iY + pDraw->y) * pDraw->iCanvasWidth) + pDraw->iX) * 3), pDraw->pPixels, pDraw->iWidth*3);
}

[bitbank2]

I'll check on this today.

[bitbank2]

ok, I found and fixed the palette problems. The first issue was improper handling of a local color palette. I also slightly changed the behavior of the cooked output. When you pass a full framebuffer and don't pass a GIFDRAW callback, it will render the cooked pixels into the framebuffer after the native (8-bit) frame. This means that for RGB565 output, the framebuffer needs to be 3 x (iCanvasWidth * iCanvasHeight).

[JarvyJ]

When you pass a full framebuffer and don't pass a GIFDRAW callback, it will render the cooked pixels into the framebuffer after the native (8-bit) frame

Oh man, that "fully cooked" mode is really nice. Took me a minute to realize the changes aren't in the Turbo mode decoder (the colors are still a little stripey there). but in non-Turbo mode, things are working great!

[bitbank2]

it is confusing - too many options. Do you think Turbo mode behavior should change?

[JarvyJ]

Do you think Turbo mode behavior should change?

Yeah, I think so. IMO, turbo mode should output the same as normal mode, just quicker if you have more RAM (assuming that's all technically possible).

I do feel like to maybe clear things up, the way the user interacts with the pixels should be set in ucDrawType. Then you could add this "fully cooked" (or maybe FULL_FRAME) mode as a ucDrawType:

ucDrawType	pFrameBuffer size	GIFDRAW callback
GIF_DRAW_RAW	not set	Has to handle disposal/transparency for pixels each line
GIF_DRAW_COOKED	width * (height+3) - from allocFrameBuf	Gets updated pixels per line
GIF_DRAW_FULL_FRAME	width*height*(bpp+1)	NULL - new frame is in pFrameBuffer[width*height]

(some of this might be wrong, it's just what I pieced together, but hopefully shows how I think about the library)

Updating allocFrameBuf to set the ideal pFrameBuffer based on options would probably be good. Also, for user friendliness, you could potentially have a warning if they select GIF_DRAW_FULL_FRAME and have a GIFDRAW callback set.

Potentially you could have another buffer (fullFrameBuffer) that is just width*height*bpp where the fully cooked pixels are (or another pointer that goes straight there).

Honestly not sure about performance impact of any of this, just some ideas on how to maybe make all the options less confusing while still targeting all the use cases.

---

When I first started trying out the library I was a little confused about how the GIFDRAW callback worked, mostly due to this line in the README and I think all the examples show it being set, even if to just an empty function:

I also added support for 32-bpp output and the GIFDraw callback is now optional.

Bear in mind, I was trying to use the library on Linux trying to get a full 32bpp frame (and ended up copying over the full frame via the GIFDRAW callback) and not the line-by-line mode needed on microcontrollers. So maybe coming at it from a different viewpoint than others.

[bitbank2]

Part of the issue of getting correct output with GIF is that it was designed to generate RGB888 output from each frame. This output was accumulated and could include pixels from frames with different palette entries. This means that in order to display correctly, the full frame of "cooked" output needs to exist so that it can accumulate changes of any of the possible 16 million colors. When decoding a GIF animation and only feeding it line by line to the caller, if the GIF makes use of a local color palette, then the output image will become full of wrong colors. This recent change where I do a full frame decode if "cooked" is selected and pFrameBuffer exists is the only current way to get completely correct output. If you "optimize" your GIF and remove local color tables, then the line-by-line method can still look correct. With this info, do you have any other ideas about all of the permutations of output?

[JarvyJ]

This recent change where I do a full frame decode if "cooked" is selected and pFrameBuffer exists is the only current way to get completely correct output.

I feel it's important to keep all your users on MCUs with a working gif decoder, even if it doesn't handle all gifs. While I'm not a fan of "fix it in docs" - if folks come across gifs with local color palettes on a low memory system, they could use something like gifsicle with its --color option to eliminate the local color tables. Also not entirely sure how common the local color table gifs are.

As far as permutations of options, I'm thinking something like:

1. RAW
2. COOKED
3. HIGH_ACCURACY

You could maybe have the draw type associated with the amount of memory to keep things less confusing. Maybe even have a TURBO_RAW or TURBO_COOKED if those modes make sense where folks would provide the extra buffer.

Regardless, I do feel like this is quite the odd situation to be in...