[PATCH 0/6] MR3359: gdiplus: Set GIF properties correctly.
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index b5f86c7e1b9..9e0b7909ee0 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -3974,13 +3974,15 @@ static const struct tiff_data }; #include "poppack.h" +struct property_test_data +{ + ULONG type, id, length; + const BYTE value[32]; +}; + static void test_tiff_properties(void) { - static const struct test_data - { - ULONG type, id, length; - const BYTE value[24]; - } td[31] = + static const struct property_test_data td[31] = { { PropertyTagTypeShort, 0xff, 2, { 0 } }, { PropertyTagTypeLong, 0x100, 4, { 1 } }, @@ -4098,11 +4100,7 @@ static void test_tiff_properties(void) static void test_GdipGetAllPropertyItems(void) { - static const struct test_data - { - ULONG type, id, length; - BYTE value[32]; - } td[16] = + static const struct property_test_data td[16] = { { PropertyTagTypeLong, 0xfe, 4, { 0 } }, { PropertyTagTypeShort, 0x100, 2, { 1 } }, @@ -4842,11 +4840,7 @@ static const BYTE animatedgif[] = { static void test_gif_properties(void) { - static const struct test_data - { - ULONG type, id, length; - const BYTE value[13]; - } td[] = + static const struct property_test_data td[] = { { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,20,0,0,0 } }, { PropertyTagTypeASCII, PropertyTagExifUserComment, 13, { 'H','e','l','l','o',' ','W','o','r','l','d','!',0 } }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 208 +++++++++++++++++++++---------------- 1 file changed, 121 insertions(+), 87 deletions(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 9e0b7909ee0..980bf1582a9 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -4840,130 +4840,164 @@ static const BYTE animatedgif[] = { static void test_gif_properties(void) { - static const struct property_test_data td[] = + static const struct property_test_data animatedgif_props[] = { { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,20,0,0,0 } }, - { PropertyTagTypeASCII, PropertyTagExifUserComment, 13, { 'H','e','l','l','o',' ','W','o','r','l','d','!',0 } }, + { PropertyTagTypeASCII, PropertyTagExifUserComment, 13, "Hello World!" }, { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 5,0 } }, { PropertyTagTypeByte, PropertyTagGlobalPalette, 12, { 0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c } }, { PropertyTagTypeByte, PropertyTagIndexBackground, 1, { 2 } }, { PropertyTagTypeByte, PropertyTagIndexTransparent, 1, { 8 } } }; + + static const struct test_data { + const BYTE *image_data; + size_t image_size; + const struct property_test_data *prop_item; + size_t prop_count; + UINT frame_count; + } td[] = + { +#define giftest(img, prop, frames) { img, sizeof(img), prop, ARRAY_SIZE(prop), frames } + giftest(animatedgif, animatedgif_props, 2), +#undef giftest + }; + GpStatus status; GpImage *image; GUID guid; - UINT dim_count, frame_count, prop_count, prop_size, i; + UINT dim_count, frame_count, prop_count, prop_size, i, j; UINT total_size, total_count; PROPID *prop_id; PropertyItem *prop_item; const char *item_data; - image = load_image(animatedgif, sizeof(animatedgif), TRUE, FALSE); - if (!image) /* XP fails to load this GIF image */ + for (i = 0; i < ARRAY_SIZE(td); i++) { - trace("Failed to load GIF image data\n"); - return; - } + winetest_push_context("test %u", i); - status = GdipImageGetFrameDimensionsCount(image, &dim_count); - expect(Ok, status); - expect(1, dim_count); + image = load_image(td[i].image_data, td[i].image_size, TRUE, FALSE); + if (!image) /* XP fails to load this GIF image */ + { + trace("Failed to load GIF image data\n"); + winetest_pop_context(); + continue; + } - status = GdipImageGetFrameDimensionsList(image, &guid, 1); - expect(Ok, status); - expect_guid(&FrameDimensionTime, &guid, __LINE__, FALSE); + status = GdipImageGetFrameDimensionsCount(image, &dim_count); + expect(Ok, status); + expect(1, dim_count); - status = GdipImageGetFrameCount(image, &guid, &frame_count); - expect(Ok, status); - expect(2, frame_count); + status = GdipImageGetFrameDimensionsList(image, &guid, 1); + expect(Ok, status); + expect_guid(&FrameDimensionTime, &guid, __LINE__, FALSE); - status = GdipImageSelectActiveFrame(image, &guid, 1); - expect(Ok, status); + status = GdipImageGetFrameCount(image, &guid, &frame_count); + expect(Ok, status); + expect(td[i].frame_count, frame_count); - status = GdipGetPropertyCount(image, &prop_count); - expect(Ok, status); - ok(prop_count == ARRAY_SIZE(td) || broken(prop_count == 1) /* before win7 */, - "expected property count %u, got %u\n", (UINT) ARRAY_SIZE(td), prop_count); + status = GdipImageSelectActiveFrame(image, &guid, td[i].frame_count - 1); + expect(Ok, status); - if (prop_count != ARRAY_SIZE(td)) - { - GdipDisposeImage(image); - return; - } + status = GdipGetPropertyCount(image, &prop_count); + expect(Ok, status); + ok(prop_count == td[i].prop_count || broken(prop_count == 1) /* before win7 */, + "expected property count %u, got %u\n", (UINT) td[i].prop_count, prop_count); - prop_id = HeapAlloc(GetProcessHeap(), 0, prop_count * sizeof(*prop_id)); + if (prop_count != td[i].prop_count) + { + win_skip("Property count mismatch.\n"); + GdipDisposeImage(image); + winetest_pop_context(); + continue; + } - status = GdipGetPropertyIdList(image, prop_count, prop_id); - expect(Ok, status); + prop_id = HeapAlloc(GetProcessHeap(), 0, prop_count * sizeof(*prop_id)); - prop_size = 0; - for (i = 0; i < prop_count; i++) - { - UINT size; - status = GdipGetPropertyItemSize(image, prop_id[i], &size); + status = GdipGetPropertyIdList(image, prop_count, prop_id); expect(Ok, status); - if (status != Ok) break; - ok(size > sizeof(*prop_item), "%u: too small item length %u\n", i, size); - - prop_size += size; - prop_item = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size); - status = GdipGetPropertyItem(image, prop_id[i], size, prop_item); - expect(Ok, status); - ok(prop_item->value == prop_item + 1, "expected item->value %p, got %p\n", prop_item + 1, prop_item->value); - ok(td[i].type == prop_item->type, - "%u: expected type %lu, got %u\n", i, td[i].type, prop_item->type); - ok(td[i].id == prop_item->id, "%u: expected id %#lx, got %#lx\n", i, td[i].id, prop_item->id); - size -= sizeof(*prop_item); - ok(prop_item->length == size, "%u: expected length %u, got %lu\n", i, size, prop_item->length); - ok(td[i].length == prop_item->length, "%u: expected length %lu, got %lu\n", i, td[i].length, prop_item->length); - if (td[i].length == prop_item->length) + prop_size = 0; + for (j = 0; j < prop_count; j++) { - int match = memcmp(td[i].value, prop_item->value, td[i].length) == 0; - ok(match, "%u: data mismatch\n", i); - if (!match) - trace("id %#lx:%s\n", prop_item->id, dbgstr_hexdata(prop_item->value, prop_item->length)); + UINT size; + + winetest_push_context("property %u", j); + + status = GdipGetPropertyItemSize(image, prop_id[j], &size); + expect(Ok, status); + if (status != Ok) break; + ok(size > sizeof(*prop_item), "too small item length %u\n", size); + + prop_size += size; + + prop_item = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size); + status = GdipGetPropertyItem(image, prop_id[j], size, prop_item); + expect(Ok, status); + ok(prop_item->value == prop_item + 1, "expected item->value %p, got %p\n", prop_item + 1, prop_item->value); + ok(td[i].prop_item[j].type == prop_item->type, + "expected type %lu, got %u\n", td[i].prop_item[j].type, prop_item->type); + ok(td[i].prop_item[j].id == prop_item->id, "expected id %#lx, got %#lx\n", td[i].prop_item[j].id, prop_item->id); + size -= sizeof(*prop_item); + ok(prop_item->length == size, "expected length %u, got %lu\n", size, prop_item->length); + ok(td[i].prop_item[j].length == prop_item->length, "expected length %lu, got %lu\n", td[i].prop_item[j].length, prop_item->length); + if (td[i].prop_item[j].length == prop_item->length) + { + int match = memcmp(td[i].prop_item[j].value, prop_item->value, td[i].prop_item[j].length) == 0; + ok(match, "data mismatch\n"); + if (!match) + trace("id %#lx:%s\n", prop_item->id, dbgstr_hexdata(prop_item->value, prop_item->length)); + } + HeapFree(GetProcessHeap(), 0, prop_item); + + winetest_pop_context(); } - HeapFree(GetProcessHeap(), 0, prop_item); - } - HeapFree(GetProcessHeap(), 0, prop_id); + HeapFree(GetProcessHeap(), 0, prop_id); - total_size = 0xdeadbeef; - total_count = 0xdeadbeef; - status = GdipGetPropertySize(image, &total_size, &total_count); - expect(Ok, status); - ok(prop_count == total_count, - "expected total property count %u, got %u\n", prop_count, total_count); - ok(prop_size == total_size, - "expected total property size %u, got %u\n", prop_size, total_size); + total_size = 0xdeadbeef; + total_count = 0xdeadbeef; + status = GdipGetPropertySize(image, &total_size, &total_count); + expect(Ok, status); + ok(prop_count == total_count, + "expected total property count %u, got %u\n", prop_count, total_count); + ok(prop_size == total_size, + "expected total property size %u, got %u\n", prop_size, total_size); - prop_item = HeapAlloc(GetProcessHeap(), 0, prop_size); - status = GdipGetAllPropertyItems(image, prop_size, prop_count, prop_item); - expect(Ok, status); + prop_item = HeapAlloc(GetProcessHeap(), 0, prop_size); - item_data = (const char *)(prop_item + prop_count); - for (i = 0; i < prop_count; i++) - { - ok(prop_item[i].value == item_data, "%u: expected value %p, got %p\n", - i, item_data, prop_item[i].value); - ok(td[i].type == prop_item[i].type, - "%u: expected type %lu, got %u\n", i, td[i].type, prop_item[i].type); - ok(td[i].id == prop_item[i].id, "%u: expected id %#lx, got %#lx\n", i, td[i].id, prop_item[i].id); - ok(td[i].length == prop_item[i].length, "%u: expected length %lu, got %lu\n", i, td[i].length, prop_item[i].length); - if (td[i].length == prop_item[i].length) + status = GdipGetAllPropertyItems(image, prop_size, prop_count, prop_item); + expect(Ok, status); + + item_data = (const char *)(prop_item + prop_count); + for (j = 0; j < prop_count; j++) { - int match = memcmp(td[i].value, prop_item[i].value, td[i].length) == 0; - ok(match, "%u: data mismatch\n", i); - if (!match) - trace("id %#lx:%s\n", prop_item[i].id, dbgstr_hexdata(prop_item[i].value, prop_item[i].length)); + winetest_push_context("property %u", j); + + ok(prop_item[j].value == item_data, "expected value %p, got %p\n", + item_data, prop_item[j].value); + ok(td[i].prop_item[j].type == prop_item[j].type, + "expected type %lu, got %u\n", td[i].prop_item[j].type, prop_item[j].type); + ok(td[i].prop_item[j].id == prop_item[j].id, "expected id %#lx, got %#lx\n", td[i].prop_item[j].id, prop_item[j].id); + ok(td[i].prop_item[j].length == prop_item[j].length, "expected length %lu, got %lu\n", td[i].prop_item[j].length, prop_item[j].length); + if (td[i].prop_item[j].length == prop_item[j].length) + { + int match = memcmp(td[i].prop_item[j].value, prop_item[j].value, td[i].prop_item[j].length) == 0; + ok(match, "data mismatch\n"); + if (!match) + trace("id %#lx:%s\n", prop_item[j].id, dbgstr_hexdata(prop_item[j].value, prop_item[j].length)); + } + item_data += prop_item[j].length; + + winetest_pop_context(); } - item_data += prop_item[i].length; - } - HeapFree(GetProcessHeap(), 0, prop_item); + HeapFree(GetProcessHeap(), 0, prop_item); - GdipDisposeImage(image); + GdipDisposeImage(image); + + winetest_pop_context(); + } } static void test_ARGB_conversion(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/image.c | 11 +++++++---- dlls/gdiplus/tests/image.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 4ba6b1efd95..a1ad60ecce3 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -3171,11 +3171,14 @@ static PropertyItem *get_gif_loopcount(IWICMetadataReader *reader) static PropertyItem *get_gif_background(IWICMetadataReader *reader) { - PropertyItem *background; + PropertyItem *background = NULL; - background = get_property(reader, &GUID_MetadataFormatLSD, L"BackgroundColorIndex"); - if (background) - background->id = PropertyTagIndexBackground; + if (get_bool_property(reader, &GUID_MetadataFormatLSD, L"GlobalColorTableFlag")) + { + background = get_property(reader, &GUID_MetadataFormatLSD, L"BackgroundColorIndex"); + if (background) + background->id = PropertyTagIndexBackground; + } return background; } diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 980bf1582a9..c014c8d4619 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -4838,6 +4838,27 @@ static const BYTE animatedgif[] = { 0x21,0x01,0x0C,'p','l','a','i','n','t','e','x','t',' ','#','2',0x00,0x3B }; +static const BYTE gif_2frame_global_pal[] = { +'G','I','F','8','7','a', 0x01,0x00, 0x01,0x00, 0xa1, 0x02, 0x00, +0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c, +0x21,0xF9,0x04, 0x00,0x0A,0x00,0x08, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, +0x21,0xF9,0x04, 0x00,0x14,0x00,0x08, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, 0x3b +}; + +static const BYTE gif_2frame_no_pal[] = { +'G','I','F','8','7','a', 0x01,0x00, 0x01,0x00, 0x21, 0x02, 0x00, +0x21,0xF9,0x04, 0x00,0x0A,0x00,0x08, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, +0x21,0xF9,0x04, 0x00,0x14,0x00,0x08, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, 0x3b +}; + static void test_gif_properties(void) { static const struct property_test_data animatedgif_props[] = @@ -4849,6 +4870,18 @@ static void test_gif_properties(void) { PropertyTagTypeByte, PropertyTagIndexBackground, 1, { 2 } }, { PropertyTagTypeByte, PropertyTagIndexTransparent, 1, { 8 } } }; + static const struct property_test_data gif_2frame_global_pal_props[] = + { + { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,20,0,0,0 } }, + { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, + { PropertyTagTypeByte, PropertyTagGlobalPalette, 12, { 0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c } }, + { PropertyTagTypeByte, PropertyTagIndexBackground, 1, { 2 } }, + }; + static const struct property_test_data gif_2frame_no_pal_props[] = + { + { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,20,0,0,0 } }, + { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, + }; static const struct test_data { const BYTE *image_data; @@ -4860,6 +4893,8 @@ static void test_gif_properties(void) { #define giftest(img, prop, frames) { img, sizeof(img), prop, ARRAY_SIZE(prop), frames } giftest(animatedgif, animatedgif_props, 2), + giftest(gif_2frame_global_pal, gif_2frame_global_pal_props, 2), + giftest(gif_2frame_no_pal, gif_2frame_no_pal_props, 2), #undef giftest }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
From: Jeff Smith <whydoubt(a)gmail.com> These properties are expected to be present even if frame count is 1. --- dlls/gdiplus/image.c | 36 ++++++++++++++++-------------------- dlls/gdiplus/tests/image.c | 14 +++++++++++++- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index a1ad60ecce3..7a25b1f0928 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -3301,29 +3301,25 @@ static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UI PropertyItem *transparent_idx = NULL, *loop = NULL, *palette = NULL; IWICBitmapDecoder_GetFrameCount(decoder, &frame_count); - if (frame_count > 1) + delay = heap_alloc_zero(sizeof(*delay) + frame_count * sizeof(LONG)); + if (delay) { - delay = heap_alloc_zero(sizeof(*delay) + frame_count * sizeof(LONG)); - if (delay) - { - LONG *value; + LONG *value; - delay->type = PropertyTagTypeLong; - delay->id = PropertyTagFrameDelay; - delay->length = frame_count * sizeof(LONG); - delay->value = delay + 1; + delay->type = PropertyTagTypeLong; + delay->id = PropertyTagFrameDelay; + delay->length = frame_count * sizeof(LONG); + delay->value = delay + 1; - value = delay->value; + value = delay->value; - for (i = 0; i < frame_count; i++) + for (i = 0; i < frame_count; i++) + { + hr = IWICBitmapDecoder_GetFrame(decoder, i, &frame); + if (hr == S_OK) { - hr = IWICBitmapDecoder_GetFrame(decoder, i, &frame); - if (hr == S_OK) - { - value[i] = get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Delay"); - IWICBitmapFrameDecode_Release(frame); - } - else value[i] = 0; + value[i] = get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Delay"); + IWICBitmapFrameDecode_Release(frame); } } } @@ -3342,7 +3338,7 @@ static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UI if (!comment) comment = get_gif_comment(reader); - if (frame_count > 1 && !loop) + if (!loop) loop = get_gif_loopcount(reader); if (!background) @@ -3358,7 +3354,7 @@ static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UI IWICMetadataBlockReader_Release(block_reader); } - if (frame_count > 1 && !loop) + if (!loop) { loop = heap_alloc_zero(sizeof(*loop) + sizeof(SHORT)); if (loop) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index c014c8d4619..93260c42731 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -3726,7 +3726,7 @@ static void test_image_properties(void) status = GdipGetPropertyCount(image, &prop_count); ok(status == Ok, "GdipGetPropertyCount error %d\n", status); - todo_wine_if(td[i].image_data == pngimage || td[i].image_data == jpgimage || td[i].image_data == gifimage) + todo_wine_if(td[i].image_data == pngimage || td[i].image_data == jpgimage) ok(td[i].prop_count == prop_count || (td[i].prop_count2 != ~0 && td[i].prop_count2 == prop_count), "expected property count %u or %u, got %u\n", td[i].prop_count, td[i].prop_count2, prop_count); @@ -4859,6 +4859,12 @@ static const BYTE gif_2frame_no_pal[] = { 0x02,0x02,0x44,0x01,0x00, 0x3b }; +static const BYTE gif_no_pal[] = { +'G','I','F','8','7','a', 0x01,0x00, 0x01,0x00, 0x27, 0x02, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, 0x3b +}; + static void test_gif_properties(void) { static const struct property_test_data animatedgif_props[] = @@ -4882,6 +4888,11 @@ static void test_gif_properties(void) { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,20,0,0,0 } }, { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, }; + static const struct property_test_data gif_no_pal_props[] = + { + { PropertyTagTypeLong, PropertyTagFrameDelay, 4, { 0,0,0,0 } }, + { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, + }; static const struct test_data { const BYTE *image_data; @@ -4895,6 +4906,7 @@ static void test_gif_properties(void) giftest(animatedgif, animatedgif_props, 2), giftest(gif_2frame_global_pal, gif_2frame_global_pal_props, 2), giftest(gif_2frame_no_pal, gif_2frame_no_pal_props, 2), + giftest(gif_no_pal, gif_no_pal_props, 1), #undef giftest }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/image.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 7a25b1f0928..1cbad7742b5 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -3250,14 +3250,13 @@ static PropertyItem *get_gif_transparent_idx(IWICMetadataReader *reader) return index; } -static LONG get_gif_frame_property(IWICBitmapFrameDecode *frame, const GUID *format, const WCHAR *property) +static void get_gif_frame_property(IWICBitmapFrameDecode *frame, const GUID *format, const WCHAR *property, LONG *value) { HRESULT hr; IWICMetadataBlockReader *block_reader; IWICMetadataReader *reader; UINT block_count, i; PropertyItem *prop; - LONG value = 0; hr = IWICBitmapFrameDecode_QueryInterface(frame, &IID_IWICMetadataBlockReader, (void **)&block_reader); if (hr == S_OK) @@ -3274,9 +3273,9 @@ static LONG get_gif_frame_property(IWICBitmapFrameDecode *frame, const GUID *for if (prop) { if (prop->type == PropertyTagTypeByte && prop->length == 1) - value = *(BYTE *)prop->value; + *value = *(BYTE *)prop->value; else if (prop->type == PropertyTagTypeShort && prop->length == 2) - value = *(SHORT *)prop->value; + *value = *(SHORT *)prop->value; heap_free(prop); } @@ -3286,8 +3285,6 @@ static LONG get_gif_frame_property(IWICBitmapFrameDecode *frame, const GUID *for } IWICMetadataBlockReader_Release(block_reader); } - - return value; } static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UINT active_frame) @@ -3318,7 +3315,7 @@ static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UI hr = IWICBitmapDecoder_GetFrame(decoder, i, &frame); if (hr == S_OK) { - value[i] = get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Delay"); + get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Delay", &value[i]); IWICBitmapFrameDecode_Release(frame); } } @@ -3832,8 +3829,11 @@ static GpStatus select_frame_wic(GpImage *image, UINT active_frame) static HRESULT get_gif_frame_rect(IWICBitmapFrameDecode *frame, UINT *left, UINT *top, UINT *width, UINT *height) { - *left = get_gif_frame_property(frame, &GUID_MetadataFormatIMD, L"Left"); - *top = get_gif_frame_property(frame, &GUID_MetadataFormatIMD, L"Top"); + LONG frame_left = 0, frame_top = 0; + get_gif_frame_property(frame, &GUID_MetadataFormatIMD, L"Left", &frame_left); + get_gif_frame_property(frame, &GUID_MetadataFormatIMD, L"Top", &frame_top); + *left = frame_left; + *top = frame_top; return IWICBitmapFrameDecode_GetSize(frame, width, height); } @@ -3907,7 +3907,8 @@ static GpStatus select_frame_gif(GpImage* image, UINT active_frame) { GpBitmap *bitmap = (GpBitmap*)image; IWICBitmapFrameDecode *frame; - int cur_frame=0, disposal; + int cur_frame=0; + LONG disposal; BOOL bgcolor_set = FALSE; DWORD bgcolor = 0; HRESULT hr; @@ -3916,7 +3917,8 @@ static GpStatus select_frame_gif(GpImage* image, UINT active_frame) hr = IWICBitmapDecoder_GetFrame(bitmap->image.decoder, image->current_frame, &frame); if(FAILED(hr)) return hresult_to_status(hr); - disposal = get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Disposal"); + disposal = 0; + get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Disposal", &disposal); IWICBitmapFrameDecode_Release(frame); if(disposal == GIF_DISPOSE_RESTORE_TO_BKGND) @@ -3929,7 +3931,8 @@ static GpStatus select_frame_gif(GpImage* image, UINT active_frame) hr = IWICBitmapDecoder_GetFrame(bitmap->image.decoder, cur_frame, &frame); if(FAILED(hr)) return hresult_to_status(hr); - disposal = get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Disposal"); + disposal = 0; + get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Disposal", &disposal); if(disposal==GIF_DISPOSE_UNSPECIFIED || disposal==GIF_DISPOSE_DO_NOT_DISPOSE) { hr = blit_gif_frame(bitmap, frame, cur_frame==0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/image.c | 4 +++- dlls/gdiplus/tests/image.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 1cbad7742b5..56a3cfa1d18 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -3302,6 +3302,7 @@ static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UI if (delay) { LONG *value; + LONG frame_delay = 0; delay->type = PropertyTagTypeLong; delay->id = PropertyTagFrameDelay; @@ -3315,9 +3316,10 @@ static void gif_metadata_reader(GpBitmap *bitmap, IWICBitmapDecoder *decoder, UI hr = IWICBitmapDecoder_GetFrame(decoder, i, &frame); if (hr == S_OK) { - get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Delay", &value[i]); + get_gif_frame_property(frame, &GUID_MetadataFormatGCE, L"Delay", &frame_delay); IWICBitmapFrameDecode_Release(frame); } + value[i] = frame_delay; } } diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 93260c42731..5546f48a998 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -4859,6 +4859,24 @@ static const BYTE gif_2frame_no_pal[] = { 0x02,0x02,0x44,0x01,0x00, 0x3b }; +static const BYTE gif_2frame_missing_gce1[] = { +'G','I','F','8','7','a', 0x01,0x00, 0x01,0x00, 0x21, 0x02, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, +0x21,0xF9,0x04, 0x00,0x14,0x00,0x08, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, 0x3b +}; + +static const BYTE gif_2frame_missing_gce2[] = { +'G','I','F','8','7','a', 0x01,0x00, 0x01,0x00, 0x21, 0x02, 0x00, +0x21,0xF9,0x04, 0x00,0x0A,0x00,0x08, 0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, +0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, +0x02,0x02,0x44,0x01,0x00, 0x3b +}; + static const BYTE gif_no_pal[] = { 'G','I','F','8','7','a', 0x01,0x00, 0x01,0x00, 0x27, 0x02, 0x00, 0x2c, 0x00,0x00, 0x00,0x00, 0x01,0x00, 0x01,0x00, 0x01, @@ -4888,6 +4906,16 @@ static void test_gif_properties(void) { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,20,0,0,0 } }, { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, }; + static const struct property_test_data gif_2frame_missing_gce1_props[] = + { + { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 0,0,0,0,20,0,0,0 } }, + { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, + }; + static const struct property_test_data gif_2frame_missing_gce2_props[] = + { + { PropertyTagTypeLong, PropertyTagFrameDelay, 8, { 10,0,0,0,10,0,0,0 } }, + { PropertyTagTypeShort, PropertyTagLoopCount, 2, { 1,0 } }, + }; static const struct property_test_data gif_no_pal_props[] = { { PropertyTagTypeLong, PropertyTagFrameDelay, 4, { 0,0,0,0 } }, @@ -4906,6 +4934,8 @@ static void test_gif_properties(void) giftest(animatedgif, animatedgif_props, 2), giftest(gif_2frame_global_pal, gif_2frame_global_pal_props, 2), giftest(gif_2frame_no_pal, gif_2frame_no_pal_props, 2), + giftest(gif_2frame_missing_gce1, gif_2frame_missing_gce1_props, 2), + giftest(gif_2frame_missing_gce2, gif_2frame_missing_gce2_props, 2), giftest(gif_no_pal, gif_no_pal_props, 1), #undef giftest }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
Esme Povirk (@madewokherd) commented about dlls/gdiplus/image.c:
return index; }
-static LONG get_gif_frame_property(IWICBitmapFrameDecode *frame, const GUID *format, const WCHAR *property) +static void get_gif_frame_property(IWICBitmapFrameDecode *frame, const GUID *format, const WCHAR *property, LONG *value)
I don't understand the redesign here. I guess this would enable default values other than 0, but afaict the code doesn't make use of that. Am I missing something? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3359#note_39757
On Fri Jul 21 20:10:08 2023 +0000, Esme Povirk wrote: > I don't understand the redesign here. I guess this would enable default > values other than 0, but afaict the code doesn't make use of that. Am I > missing something? Assuming the property is not present... - Currently `x = get_gif_frame_property(frm, fmt, prop);` will result in `x` being 0. - With this change, `get_gif_frame_property(frm, fmt, prop, &x);` will leave `x` unchanged. - Alternately (and more repetitiously) this could be implemented with a default parameter, and pass in the original value: `x = get_gif_frame_property(frm, fmt, prop, x);` This functionality is made use of in the following commit. To create the data for `PropertyTagFrameDelay`, we loop over a set of frames, and create a list of delays, one per frame. If a delay is not specified for a frame, it will get the same delay as the previous frame. So if we have (GCE w/ delay:10) (frame 1 image) (frame 2 image), we should get [10, 10] for the delays, but we currently get [10, 0]. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3359#note_39779
On Fri Jul 21 22:45:28 2023 +0000, Jeffrey Smith wrote:
Assuming the property is not present... - Currently `x = get_gif_frame_property(frm, fmt, prop);` will result in `x` being 0. - With this change, `get_gif_frame_property(frm, fmt, prop, &x);` will leave `x` unchanged. - Alternately (and more repetitiously) this could be implemented with a default parameter, and pass in the original value: `x = get_gif_frame_property(frm, fmt, prop, x);` This functionality is made use of in the following commit. To create the data for `PropertyTagFrameDelay`, we loop over a set of frames, and create a list of delays, one per frame. If a delay is not specified for a frame, it will get the same delay as the previous frame. So if we have (GCE w/ delay:10) (frame 1 image) (frame 2 image), we should get [10, 10] for the delays, but we currently get [10, 0]. I see, thank you for explaining.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3359#note_39839
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3359
participants (3)
-
Esme Povirk (＠madewokherd) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)