[PATCH v3 0/2] MR3344: windowscodecs: support GIF with no color table
Decoding a GIF with no color table currently crashes on Wine. Windows supports such files, and the GIF spec explicitly allows it. -- v3: windowscodecs: Allow GIF with no color table. windowscodecs: Factor out common GIF palette copying logic. https://gitlab.winehq.org/wine/wine/-/merge_requests/3344
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/windowscodecs/gifformat.c | 92 +++++++++++++++------------------- 1 file changed, 40 insertions(+), 52 deletions(-) diff --git a/dlls/windowscodecs/gifformat.c b/dlls/windowscodecs/gifformat.c index f0048d12da9..11b2ccf6aea 100644 --- a/dlls/windowscodecs/gifformat.c +++ b/dlls/windowscodecs/gifformat.c @@ -701,14 +701,49 @@ static HRESULT WINAPI GifFrameDecode_GetResolution(IWICBitmapFrameDecode *iface, return S_OK; } +static void copy_palette(ColorMapObject *cm, Extensions *extensions, int count, WICColor *colors) +{ + int i; + + if (cm) + { + for (i = 0; i < count; i++) + { + colors[i] = 0xff000000 | /* alpha */ + cm->Colors[i].Red << 16 | + cm->Colors[i].Green << 8 | + cm->Colors[i].Blue; + } + } + else + { + colors[0] = 0xff000000; + colors[1] = 0xffffffff; + for (i = 2; i < count; i++) + colors[i] = 0xff000000; + } + + /* look for the transparent color extension */ + for (i = 0; i < extensions->ExtensionBlockCount; i++) + { + ExtensionBlock *eb = extensions->ExtensionBlocks + i; + if (eb->Function == GRAPHICS_EXT_FUNC_CODE && + eb->ByteCount == 8 && eb->Bytes[3] & 1) + { + int trans = (unsigned char)eb->Bytes[6]; + colors[trans] &= 0xffffff; /* set alpha to 0 */ + break; + } + } +} + static HRESULT WINAPI GifFrameDecode_CopyPalette(IWICBitmapFrameDecode *iface, IWICPalette *pIPalette) { GifFrameDecode *This = impl_from_IWICBitmapFrameDecode(iface); WICColor colors[256]; ColorMapObject *cm = This->frame->ImageDesc.ColorMap; - int i, trans; - ExtensionBlock *eb; + TRACE("(%p,%p)\n", iface, pIPalette); if (!cm) cm = This->parent->gif->SColorMap; @@ -719,24 +754,7 @@ static HRESULT WINAPI GifFrameDecode_CopyPalette(IWICBitmapFrameDecode *iface, return E_FAIL; } - for (i = 0; i < cm->ColorCount; i++) { - colors[i] = 0xff000000| /* alpha */ - cm->Colors[i].Red << 16| - cm->Colors[i].Green << 8| - cm->Colors[i].Blue; - } - - /* look for the transparent color extension */ - for (i = 0; i < This->frame->Extensions.ExtensionBlockCount; ++i) { - eb = This->frame->Extensions.ExtensionBlocks + i; - if (eb->Function == GRAPHICS_EXT_FUNC_CODE && eb->ByteCount == 8) { - if (eb->Bytes[3] & 1) { - trans = (unsigned char)eb->Bytes[6]; - colors[trans] &= 0xffffff; /* set alpha to 0 */ - break; - } - } - } + copy_palette(cm, &This->frame->Extensions, cm->ColorCount, colors); return IWICPalette_InitializeCustom(pIPalette, colors, cm->ColorCount); } @@ -1170,8 +1188,7 @@ static HRESULT WINAPI GifDecoder_CopyPalette(IWICBitmapDecoder *iface, IWICPalet GifDecoder *This = impl_from_IWICBitmapDecoder(iface); WICColor colors[256]; ColorMapObject *cm; - int i, trans, count; - ExtensionBlock *eb; + int count; TRACE("(%p,%p)\n", iface, palette); @@ -1187,41 +1204,12 @@ static HRESULT WINAPI GifDecoder_CopyPalette(IWICBitmapDecoder *iface, IWICPalet return E_FAIL; } - for (i = 0; i < cm->ColorCount; i++) - { - colors[i] = 0xff000000 | /* alpha */ - cm->Colors[i].Red << 16 | - cm->Colors[i].Green << 8 | - cm->Colors[i].Blue; - } - count = cm->ColorCount; } else - { - colors[0] = 0xff000000; - colors[1] = 0xffffffff; - - for (i = 2; i < 256; i++) - colors[i] = 0xff000000; - count = 256; - } - /* look for the transparent color extension */ - for (i = 0; i < This->gif->SavedImages[This->current_frame].Extensions.ExtensionBlockCount; i++) - { - eb = This->gif->SavedImages[This->current_frame].Extensions.ExtensionBlocks + i; - if (eb->Function == GRAPHICS_EXT_FUNC_CODE && eb->ByteCount == 8) - { - if (eb->Bytes[3] & 1) - { - trans = (unsigned char)eb->Bytes[6]; - colors[trans] &= 0xffffff; /* set alpha to 0 */ - break; - } - } - } + copy_palette(cm, &This->gif->SavedImages[This->current_frame].Extensions, count, colors); return IWICPalette_InitializeCustom(palette, colors, count); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3344
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/windowscodecs/gifformat.c | 32 +++++++------ dlls/windowscodecs/tests/gifformat.c | 70 ++++++++++++++++++++++++++++ dlls/windowscodecs/ungif.c | 1 + dlls/windowscodecs/ungif.h | 1 + 4 files changed, 89 insertions(+), 15 deletions(-) diff --git a/dlls/windowscodecs/gifformat.c b/dlls/windowscodecs/gifformat.c index 11b2ccf6aea..152c51eac76 100644 --- a/dlls/windowscodecs/gifformat.c +++ b/dlls/windowscodecs/gifformat.c @@ -743,20 +743,27 @@ static HRESULT WINAPI GifFrameDecode_CopyPalette(IWICBitmapFrameDecode *iface, GifFrameDecode *This = impl_from_IWICBitmapFrameDecode(iface); WICColor colors[256]; ColorMapObject *cm = This->frame->ImageDesc.ColorMap; + int count; TRACE("(%p,%p)\n", iface, pIPalette); - if (!cm) cm = This->parent->gif->SColorMap; + if (cm) + count = cm->ColorCount; + else + { + cm = This->parent->gif->SColorMap; + count = This->parent->gif->SColorTableSize; + } - if (cm->ColorCount > 256) + if (count > 256) { - ERR("GIF contains %i colors???\n", cm->ColorCount); + ERR("GIF contains %i colors???\n", count); return E_FAIL; } - copy_palette(cm, &This->frame->Extensions, cm->ColorCount, colors); + copy_palette(cm, &This->frame->Extensions, count, colors); - return IWICPalette_InitializeCustom(pIPalette, colors, cm->ColorCount); + return IWICPalette_InitializeCustom(pIPalette, colors, count); } static HRESULT copy_interlaced_pixels(const BYTE *srcbuffer, @@ -1196,18 +1203,13 @@ static HRESULT WINAPI GifDecoder_CopyPalette(IWICBitmapDecoder *iface, IWICPalet return WINCODEC_ERR_WRONGSTATE; cm = This->gif->SColorMap; - if (cm) - { - if (cm->ColorCount > 256) - { - ERR("GIF contains invalid number of colors: %d\n", cm->ColorCount); - return E_FAIL; - } + count = This->gif->SColorTableSize; - count = cm->ColorCount; + if (count > 256) + { + ERR("GIF contains invalid number of colors: %d\n", count); + return E_FAIL; } - else - count = 256; copy_palette(cm, &This->gif->SavedImages[This->current_frame].Extensions, count, colors); diff --git a/dlls/windowscodecs/tests/gifformat.c b/dlls/windowscodecs/tests/gifformat.c index 8488f486a11..08f8ba1c12b 100644 --- a/dlls/windowscodecs/tests/gifformat.c +++ b/dlls/windowscodecs/tests/gifformat.c @@ -57,6 +57,13 @@ static const char gif_local_palette[] = { 0x02,0x02,0x44,0x01,0x00,0x3b }; +static const char gif_no_palette[] = { +/* LSD */'G','I','F','8','7','a',0x01,0x00,0x01,0x00,0x21,0x02,0x00, +/* GCE */0x21,0xf9,0x04,0x01,0x05,0x00,0x01,0x00, /* index 1 */ +/* IMD */0x2c,0x00,0x00,0x00,0x00,0x01,0x00,0x01,0x00,0x00, +0x02,0x02,0x44,0x01,0x00,0x3b +}; + /* Generated with ImageMagick: * convert -delay 100 -size 2x2 xc:red \ * -dispose none -page +0+0 -size 2x1 xc:white \ @@ -364,6 +371,67 @@ static void test_local_gif_palette(void) IWICBitmapDecoder_Release(decoder); } +static void test_no_gif_palette(void) +{ + HRESULT hr; + IWICBitmapDecoder *decoder; + IWICBitmapFrameDecode *frame; + IWICPalette *palette; + GUID format; + UINT count, ret; + WICColor color[256]; + + decoder = create_decoder(gif_no_palette, sizeof(gif_no_palette)); + ok(decoder != 0, "Failed to load GIF image data\n"); + + hr = IWICImagingFactory_CreatePalette(factory, &palette); + ok(hr == S_OK, "CreatePalette error %#lx\n", hr); + + /* global palette */ + hr = IWICBitmapDecoder_CopyPalette(decoder, palette); + ok(hr == S_OK, "CopyPalette error %#lx\n", hr); + + hr = IWICPalette_GetColorCount(palette, &count); + ok(hr == S_OK, "GetColorCount error %#lx\n", hr); + ok(count == 4, "expected 4, got %u\n", count); + + hr = IWICPalette_GetColors(palette, count, color, &ret); + ok(hr == S_OK, "GetColors error %#lx\n", hr); + ok(ret == count, "expected %u, got %u\n", count, ret); + ok(color[0] == 0xff000000, "expected 0xff000000, got %#x\n", color[0]); + ok(color[1] == 0x00ffffff, "expected 0x00ffffff, got %#x\n", color[1]); + ok(color[2] == 0xff000000, "expected 0xff000000, got %#x\n", color[2]); + ok(color[3] == 0xff000000, "expected 0xff000000, got %#x\n", color[3]); + + /* frame palette */ + hr = IWICBitmapDecoder_GetFrame(decoder, 0, &frame); + ok(hr == S_OK, "GetFrame error %#lx\n", hr); + + hr = IWICBitmapFrameDecode_GetPixelFormat(frame, &format); + ok(hr == S_OK, "GetPixelFormat error %#lx\n", hr); + ok(IsEqualGUID(&format, &GUID_WICPixelFormat8bppIndexed), + "wrong pixel format %s\n", wine_dbgstr_guid(&format)); + + hr = IWICBitmapFrameDecode_CopyPalette(frame, palette); + ok(hr == S_OK, "CopyPalette error %#lx\n", hr); + + hr = IWICPalette_GetColorCount(palette, &count); + ok(hr == S_OK, "GetColorCount error %#lx\n", hr); + ok(count == 4, "expected 4, got %u\n", count); + + hr = IWICPalette_GetColors(palette, count, color, &ret); + ok(hr == S_OK, "GetColors error %#lx\n", hr); + ok(ret == count, "expected %u, got %u\n", count, ret); + ok(color[0] == 0xff000000, "expected 0xff000000, got %#x\n", color[0]); + ok(color[1] == 0x00ffffff, "expected 0x00ffffff, got %#x\n", color[1]); + ok(color[2] == 0xff000000, "expected 0xff000000, got %#x\n", color[2]); + ok(color[3] == 0xff000000, "expected 0xff000000, got %#x\n", color[3]); + + IWICPalette_Release(palette); + IWICBitmapFrameDecode_Release(frame); + IWICBitmapDecoder_Release(decoder); +} + static void test_gif_frame_sizes(void) { static const BYTE frame0[] = {0, 1, 0xfe, 0xfe, 2, 3, 0xfe, 0xfe}; @@ -577,6 +645,7 @@ START_TEST(gifformat) test_global_gif_palette(); test_global_gif_palette_2frames(); test_local_gif_palette(); + test_no_gif_palette(); test_gif_frame_sizes(); test_gif_notrailer(); @@ -590,6 +659,7 @@ START_TEST(gifformat) test_global_gif_palette(); test_global_gif_palette_2frames(); test_local_gif_palette(); + test_no_gif_palette(); test_gif_frame_sizes(); test_truncated_gif(); diff --git a/dlls/windowscodecs/ungif.c b/dlls/windowscodecs/ungif.c index 0f5ed6e25d5..1fb16e52f15 100644 --- a/dlls/windowscodecs/ungif.c +++ b/dlls/windowscodecs/ungif.c @@ -320,6 +320,7 @@ DGifGetScreenDesc(GifFileType * GifFile) { GifFile->SColorResolution = (((Buf[0] & 0x70) + 1) >> 4) + 1; SortFlag = (Buf[0] & 0x08) != 0; BitsPerPixel = (Buf[0] & 0x07) + 1; + GifFile->SColorTableSize = 1 << BitsPerPixel; GifFile->SBackGroundColor = Buf[1]; GifFile->SAspectRatio = Buf[2]; if (Buf[0] & 0x80) { /* Do we have global color map? */ diff --git a/dlls/windowscodecs/ungif.h b/dlls/windowscodecs/ungif.h index ae8269226df..f61a7eb1e16 100644 --- a/dlls/windowscodecs/ungif.h +++ b/dlls/windowscodecs/ungif.h @@ -114,6 +114,7 @@ typedef struct { typedef struct GifFileType { GifWord SWidth, SHeight, /* Screen dimensions. */ SColorResolution, /* How many colors can we generate? */ + SColorTableSize, /* Calculated color table size, even if not present */ SBackGroundColor, /* I hope you understand this one... */ SAspectRatio; /* Pixel aspect ratio, in 1/64 units, starting at 1:4. */ ColorMapObject *SColorMap; /* NULL if not exists. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3344
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134971 Your paranoid android. === debian11 (32 bit report) === Report validation errors: mshtml:dom has no test summary line (early exit of the main process?) mshtml:dom has unaccounted for todo messages mshtml:dom returned a non-zero exit code despite reporting no failures
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3344
participants (4)
-
Esme Povirk (＠madewokherd) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Marvin