[PATCH 0/4] MR3657: gdiplus: GdipCreateBitmapFromHICON fixes part 1/2
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index cbc07b877a9..a66be781128 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1646,6 +1646,28 @@ static void test_fromhicon(void) GdipDisposeImage((GpImage*)bitmap); } DestroyIcon(hIcon); + + /* 32 bpp icon */ + hbmMask = CreateBitmap(16, 16, 1, 1, bmp_bits); + ok(hbmMask != 0, "CreateBitmap failed\n"); + hbmColor = CreateBitmap(16, 16, 1, 32, bmp_bits); + ok(hbmColor != 0, "CreateBitmap failed\n"); + info = iconinfo_base; + info.hbmMask = hbmMask; + info.hbmColor = hbmColor; + hIcon = CreateIconIndirect(&info); + ok(hIcon != 0, "CreateIconIndirect failed\n"); + DeleteObject(hbmMask); + DeleteObject(hbmColor); + + stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); + expect(Ok, stat); + if(stat == Ok){ + expect_image_properties((GpImage*)bitmap, 16, 16, __LINE__); + expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); + GdipDisposeImage((GpImage*)bitmap); + } + DestroyIcon(hIcon); } /* 1x1 pixel png */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3657
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index a66be781128..103a0c56a0f 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1668,6 +1668,28 @@ static void test_fromhicon(void) GdipDisposeImage((GpImage*)bitmap); } DestroyIcon(hIcon); + + /* non-square 32 bpp icon */ + hbmMask = CreateBitmap(16, 8, 1, 1, bmp_bits); + ok(hbmMask != 0, "CreateBitmap failed\n"); + hbmColor = CreateBitmap(16, 8, 1, 32, bmp_bits); + ok(hbmColor != 0, "CreateBitmap failed\n"); + info = iconinfo_base; + info.hbmMask = hbmMask; + info.hbmColor = hbmColor; + hIcon = CreateIconIndirect(&info); + ok(hIcon != 0, "CreateIconIndirect failed\n"); + DeleteObject(hbmMask); + DeleteObject(hbmColor); + + stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); + expect(Ok, stat); + if(stat == Ok){ + expect_image_properties((GpImage*)bitmap, 16, 8, __LINE__); + expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); + GdipDisposeImage((GpImage*)bitmap); + } + DestroyIcon(hIcon); } /* 1x1 pixel png */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3657
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 103a0c56a0f..f90375478e1 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -106,6 +106,26 @@ static void expect_image_properties(GpImage *image, UINT width, UINT height, int ok_(__FILE__, line)(format == PixelFormat32bppARGB, "Expected %d, got %d\n", PixelFormat32bppARGB, format); } +static void expect_bitmap_locked_data(GpBitmap *bitmap, const BYTE *expect_bits, + UINT width, UINT height, UINT stride, int line) +{ + GpStatus stat; + BitmapData lockeddata; + + memset(&lockeddata, 0x55, sizeof(lockeddata)); + stat = GdipBitmapLockBits(bitmap, NULL, ImageLockModeRead, PixelFormat32bppARGB, &lockeddata); + ok_(__FILE__, line)(stat == Ok, "Expected %d, got %d\n", Ok, stat); + ok_(__FILE__, line)(lockeddata.Width == width, "Expected %d, got %d\n", width, lockeddata.Width); + ok_(__FILE__, line)(lockeddata.Height == height, "Expected %d, got %d\n", height, lockeddata.Height); + ok_(__FILE__, line)(lockeddata.Stride == stride, "Expected %d, got %d\n", stride, lockeddata.Stride); + ok_(__FILE__, line)(lockeddata.PixelFormat == PixelFormat32bppARGB, + "Expected %d, got %d\n", PixelFormat32bppARGB, lockeddata.PixelFormat); + todo_wine + ok_(__FILE__, line)(!memcmp(expect_bits, lockeddata.Scan0, lockeddata.Height * lockeddata.Stride), + "data mismatch\n"); + GdipBitmapUnlockBits(bitmap, &lockeddata); +} + static BOOL get_encoder_clsid(LPCWSTR mime, GUID *format, CLSID *clsid) { GpStatus status; @@ -1560,12 +1580,16 @@ static void test_testcontrol(void) static void test_fromhicon(void) { - static const BYTE bmp_bits[1024]; + BYTE bmp_bits[1024], bmp_bits_masked[1024]; HBITMAP hbmMask, hbmColor; ICONINFO info, iconinfo_base = {TRUE, 0, 0, 0, 0}; HICON hIcon; GpStatus stat; GpBitmap *bitmap = NULL; + UINT i; + + for (i = 0; i < sizeof(bmp_bits); ++i) + bmp_bits[i] = 111 * i; /* NULL */ stat = GdipCreateBitmapFromHICON(NULL, NULL); @@ -1660,11 +1684,18 @@ static void test_fromhicon(void) DeleteObject(hbmMask); DeleteObject(hbmColor); + for (i = 0; i < sizeof(bmp_bits_masked)/sizeof(ARGB); i++) + { + BYTE mask = bmp_bits[i / 8] & (1 << (7 - (i % 8))); + ((ARGB *)bmp_bits_masked)[i] = mask ? 0 : ((ARGB *)bmp_bits)[i] | 0xff000000; + } + stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); expect(Ok, stat); if(stat == Ok){ expect_image_properties((GpImage*)bitmap, 16, 16, __LINE__); expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); + expect_bitmap_locked_data(bitmap, bmp_bits_masked, 16, 16, 64, __LINE__); GdipDisposeImage((GpImage*)bitmap); } DestroyIcon(hIcon); @@ -1687,6 +1718,7 @@ static void test_fromhicon(void) if(stat == Ok){ expect_image_properties((GpImage*)bitmap, 16, 8, __LINE__); expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); + expect_bitmap_locked_data(bitmap, bmp_bits_masked, 16, 8, 64, __LINE__); GdipDisposeImage((GpImage*)bitmap); } DestroyIcon(hIcon); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3657
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/image.c | 75 +++++++++++++++++++------------------- dlls/gdiplus/tests/image.c | 1 - 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 7640c934902..36fd41244fc 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -1626,12 +1626,13 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) GpRect rect; BitmapData lockeddata; HDC screendc; - BOOL has_alpha; int x, y; BITMAPINFOHEADER bih; DWORD *src; BYTE *dst_row; DWORD *dst; + BYTE *bits; + int mask_scanlines = 0, color_scanlines = 0; TRACE("%p, %p\n", hicon, bitmap); @@ -1682,52 +1683,52 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) bih.biClrUsed = 0; bih.biClrImportant = 0; - screendc = CreateCompatibleDC(0); - GetDIBits(screendc, iinfo.hbmColor, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS); - - if (bm.bmBitsPixel == 32) + bits = heap_alloc(height * stride); + if (!bits) { - has_alpha = FALSE; - - /* If any pixel has a non-zero alpha, ignore hbmMask */ - src = (DWORD*)lockeddata.Scan0; - for (y=0; y<height && !has_alpha; y++) - for (x=0; x<width && !has_alpha; x++) - if ((*src++ & 0xff000000) != 0) - has_alpha = TRUE; + DeleteObject(iinfo.hbmColor); + DeleteObject(iinfo.hbmMask); + GdipBitmapUnlockBits(*bitmap, &lockeddata); + GdipDisposeImage(&(*bitmap)->image); + return OutOfMemory; } - else has_alpha = FALSE; - if (!has_alpha) + screendc = CreateCompatibleDC(0); + if (screendc) { - BYTE *bits = heap_alloc(height * stride); - - /* read alpha data from the mask - the mask can safely be assumed to exist */ - GetDIBits(screendc, iinfo.hbmMask, 0, height, bits, (BITMAPINFO*)&bih, DIB_RGB_COLORS); + color_scanlines = GetDIBits(screendc, iinfo.hbmColor, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS); + mask_scanlines = GetDIBits(screendc, iinfo.hbmMask, 0, height, bits, (BITMAPINFO*)&bih, DIB_RGB_COLORS); + DeleteDC(screendc); + } - src = (DWORD*)bits; - dst_row = lockeddata.Scan0; - for (y=0; y<height; y++) - { - dst = (DWORD*)dst_row; - for (x=0; x<width; x++) - { - DWORD src_value = *src++; - if (src_value) - *dst++ = 0; - else - *dst++ |= 0xff000000; - } - dst_row += lockeddata.Stride; - } + DeleteObject(iinfo.hbmColor); + DeleteObject(iinfo.hbmMask); + if (color_scanlines <= 0 || mask_scanlines <= 0) + { heap_free(bits); + GdipBitmapUnlockBits(*bitmap, &lockeddata); + GdipDisposeImage(&(*bitmap)->image); + return GenericError; } - DeleteDC(screendc); + src = (DWORD*)bits; + dst_row = lockeddata.Scan0; + for (y=0; y<height; y++) + { + dst = (DWORD*)dst_row; + for (x=0; x<width; x++) + { + DWORD src_value = *src++; + if (src_value) + *dst++ = 0; + else + *dst++ |= 0xff000000; + } + dst_row += lockeddata.Stride; + } - DeleteObject(iinfo.hbmColor); - DeleteObject(iinfo.hbmMask); + heap_free(bits); GdipBitmapUnlockBits(*bitmap, &lockeddata); diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index f90375478e1..6f497e416b0 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -120,7 +120,6 @@ static void expect_bitmap_locked_data(GpBitmap *bitmap, const BYTE *expect_bits, ok_(__FILE__, line)(lockeddata.Stride == stride, "Expected %d, got %d\n", stride, lockeddata.Stride); ok_(__FILE__, line)(lockeddata.PixelFormat == PixelFormat32bppARGB, "Expected %d, got %d\n", PixelFormat32bppARGB, lockeddata.PixelFormat); - todo_wine ok_(__FILE__, line)(!memcmp(expect_bits, lockeddata.Scan0, lockeddata.Height * lockeddata.Stride), "data mismatch\n"); GdipBitmapUnlockBits(bitmap, &lockeddata); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3657
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=136699 Your paranoid android. === debian11b (64 bit WoW report) === Report validation errors: xinput1_3:xinput crashed (c0000005)
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/image.c:
- if (src_value) - *dst++ = 0; - else - *dst++ |= 0xff000000; - } - dst_row += lockeddata.Stride; - } + DeleteObject(iinfo.hbmColor); + DeleteObject(iinfo.hbmMask);
+ if (color_scanlines <= 0 || mask_scanlines <= 0) + { heap_free(bits); + GdipBitmapUnlockBits(*bitmap, &lockeddata); + GdipDisposeImage(&(*bitmap)->image); + return GenericError; Could you verify what kind of error (if any) is returned in test case?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3657#note_43426
On Fri Aug 25 18:10:16 2023 +0000, Bartosz Kosiorek wrote:
Could you verify what kind of error (if any) is returned in test case? You could investigate how to create such test case by analysing: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/win32u/dib.c#L1221 It would be really useful, as with current implementation we are changing default behavior. @gang65 Returning an error here is an implementation detail. I don't even know that native uses GetDIBits. I do know that this implementation does, and something reasonable should be done when a bad value is returned.
Finding conditions that cause native to break and testing those is one thing, but unless you have something specific in mind already, I don't see the need for additional testing here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3657#note_43480
On Sat Aug 26 03:12:03 2023 +0000, Jeffrey Smith wrote:
@gang65 Returning an error here is an implementation detail. I don't even know that native uses GetDIBits. I do know that this implementation does, and something reasonable should be done when a bad value is returned. Finding conditions that cause native to break and testing those is one thing, but unless you have something specific in mind already, I don't see the need for additional testing here. If number of copied lines from bitmap is zero, we should keep previous behaviour and return `Ok`.
Otherwise we could introduce regression. I think returning Error without covering it with tests is very risky. More info: https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getdibi... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3657#note_43484
On Sat Aug 26 06:16:12 2023 +0000, Bartosz Kosiorek wrote:
If number of copied lines from bitmap is zero, we should keep previous behaviour and return `Ok`. Otherwise we could introduce regression. I think returning Error without covering it with tests is very risky. We even don't know what kind of error should be returned. More info: https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getdibi... AFAICT Windows won't create a bitmap with 0 height, so from the GetDIBits documentation I had believed that zero copied lines indicates a fault of some sort.
I still don't believe the fault should ever be hit, so it's rather difficult to create test case that does, but it is good practice to check fault indications. As I do not want to break on a false positive, I should probably also check GetLastError. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3657#note_43502
participants (4)
-
Bartosz Kosiorek (＠gang65) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Marvin