Fix various Gif Decoder/Encoder behaviors.

[JimBobSquarePants]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

Fixes #2288

This PR does a few things

• Ensures that Graphics Control Extension blocks are read and written correctly
• No longer clones the entire previous frame on decode and only write what we need. This was causing some gif sizes to explode.
• Fixes decode transparency flag handling.
• Ensures palette quantizer enables the whole palette
• Changes lower bit threshold for octree hack to ensure a transparent palette entry is maintained
• Allows mixing of global and local color tables per frame as per specification.
• Fixes the comparer to output the correct frames index.
• Fixes bad XMP handling

[JimBobSquarePants]

@dlemstra This might interest you.

The left shows how the changes in the decoder from this PR only decode the data from the frame. The right shows the decoded output from Magick.NET which is the same as our current behavior.

Cloning the proceeding frame like this leads to much larger output when encoding if the previous frame contains lots of color.

[dlemstra]

The right part only happens when Coalesce is called on the MagickImageCollection. ImageMagick does the same as what you see on the left side.

[JimBobSquarePants]

I'm not seeing any calls to Coalesce in our reference decoder.

public Image<TPixel> Decode<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
    where TPixel : unmanaged, ImageSharp.PixelFormats.IPixel<TPixel>
{
    Configuration configuration = options.Configuration;
    var bmpReadDefines = new BmpReadDefines
    {
        IgnoreFileSize = !this.validate
    };

    var settings = new MagickReadSettings();
    settings.FrameCount = (int)options.MaxFrames;
    settings.SetDefines(bmpReadDefines);

    using var magickImageCollection = new MagickImageCollection(stream, settings);
    var framesList = new List<ImageFrame<TPixel>>();
    foreach (IMagickImage<ushort> magicFrame in magickImageCollection)
    {
        var frame = new ImageFrame<TPixel>(configuration, magicFrame.Width, magicFrame.Height);
        framesList.Add(frame);

        MemoryGroup<TPixel> framePixels = frame.PixelBuffer.FastMemoryGroup;

        using IUnsafePixelCollection<ushort> pixels = magicFrame.GetPixelsUnsafe();
        if (magicFrame.Depth is 12 or 10 or 8 or 6 or 5 or 4 or 3 or 2 or 1)
        {
            byte[] data = pixels.ToByteArray(PixelMapping.RGBA);

            FromRgba32Bytes(configuration, data, framePixels);
        }
        else if (magicFrame.Depth is 16 or 14)
        {
            ushort[] data = pixels.ToShortArray(PixelMapping.RGBA);
            Span<byte> bytes = MemoryMarshal.Cast<ushort, byte>(data.AsSpan());
            FromRgba64Bytes(configuration, bytes, framePixels);
        }
        else
        {
            throw new InvalidOperationException();
        }
    }

    return new Image<TPixel>(configuration, new ImageMetadata(), framesList);
}

[dlemstra]

That's odd I am seeing this when I read that image inside Magick.NET and use the Immediate Window:

magickImageCollection[0].Width
500
magickImageCollection[1].Width
350
magickImageCollection[2].Width
332
magickImageCollection[3].Width
500

[JimBobSquarePants]

I'd love to figure this out properly.

[JimBobSquarePants]

Some images use a transparent index that is the same as the length of the palette.

[saucecontrol]

As far as I've seen, most viewers treat OOB transparent index as 0. That's also the behavior written into the PNG spec. Here's an example where 0 works and last index doesn't.

[JimBobSquarePants]

That's interesting. I found that the second gif in the referenced issue had a frame with a local table that contained 64 colors with a transparent index of 64. Browsers/Windows were rendering it just fine, but we were not.

Here's your input gif processed with this PR code.

[saucecontrol]

Hmm... yeah, it seems like the behavior I saw was a side-effect of the way I handle OOB values in general. I had just noticed that ImageSharp 2.1.3 was mangling that image, and I knew it had an invalid transparent index. The viewer I've been using (Microsoft GIF Animator) crashes on images like that. What's the viewer you screenshotted there?

I just looked at some web browser code, and it looks like they're just treating any OOB colors as transparent if the GCE has the transparency flag. They take an OOB transparent index as a hint that OOB color values are expected in the frame.

[JimBobSquarePants]

I use this. Stumbled upon it a few years back and it's proven incredibly useful

https://github.com/ata4/gifiddle

[saucecontrol]

Oh wow, that's great, thanks!

I see what's happening here now. Before this fix, you're not setting a transparent color because it's out of range, but you're clamping the image values into palette range, so pixels meant to be transparent are taking the last color. This was the result.

Now you're treating the last palette color as transparent and clamping OOB image values to that color, making the transparency work, but also making any pixels that were supposed to be that last palette color transparent as well. It's not creating artifacts as visible as skipping transparency, but it's also not completely correct.

In my implementation, I made the same mistake, but in the other direction. I use shared code for all animated image formats, so I followed the PNG rules of treating OOB image values as the first color in the palette. That meant I also had to treat invalid transparency index as meaning the first color was supposed to be transparent to get things to look right. Hence my initial comment 😅

So yeah, the browser behavior is better. Don't clamp the values but rather assume the palette is actually bigger than the image says.

[JimBobSquarePants]

Pushed a better fix now. I treat anything greater than the max palette index or equal to the flagged transparent index as transparent.

I only clamp following that check to ensure we don't hit OOB when reading the palette.

Thanks for your help here!

[saucecontrol]

I just happen to have run a ton of GIFs through ImageSharp this week while testing my new GIFLIB wrapper. Don't know if you've already fixed this, but this image fails to load in 2.1.3. Issue is it has a truncated XMP doc embedded (just one 255 byte segment of it)

[JimBobSquarePants]

I just happen to have run a ton of GIFs through ImageSharp this week while testing my new GIFLIB wrapper. Don't know if you've already fixed this, but this image fails to load in 2.1.3. Issue is it has a truncated XMP doc embedded (just one 255 byte segment of it)

@saucecontrol No, that one still fails (silently too!) not sure what the best fix is yet, investigating.

Edit. Fixed!