[PATCH v3 0/5] MR3621: GdipCreateBitmapFromICON fixes part 1/2
-- v3: gdiplus: Fix GdipCreateBitmapFromICON return status with mask-only icon. gdiplus/tests: Add test for mask-only icon with GdipCreateBitmapFromHICON. gdiplus/tests: Add test for cursor with GdipCreateBitmapFromHICON. gdiplus/tests: Create helper function for testing image properties. https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 0baf6eae77b..48a4652e688 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1538,7 +1538,7 @@ static void test_fromhicon(void) { static const BYTE bmp_bits[1024]; HBITMAP hbmMask, hbmColor; - ICONINFO info; + ICONINFO info, iconinfo_base = {TRUE, 0, 0, 0, 0}; HICON hIcon; GpStatus stat; GpBitmap *bitmap = NULL; @@ -1557,9 +1557,7 @@ static void test_fromhicon(void) ok(hbmMask != 0, "CreateBitmap failed\n"); hbmColor = CreateBitmap(16, 16, 1, 1, bmp_bits); ok(hbmColor != 0, "CreateBitmap failed\n"); - info.fIcon = TRUE; - info.xHotspot = 8; - info.yHotspot = 8; + info = iconinfo_base; info.hbmMask = hbmMask; info.hbmColor = hbmColor; hIcon = CreateIconIndirect(&info); @@ -1596,9 +1594,7 @@ static void test_fromhicon(void) ok(hbmMask != 0, "CreateBitmap failed\n"); hbmColor = CreateBitmap(16, 16, 1, 8, bmp_bits); ok(hbmColor != 0, "CreateBitmap failed\n"); - info.fIcon = TRUE; - info.xHotspot = 8; - info.yHotspot = 8; + info = iconinfo_base; info.hbmMask = hbmMask; info.hbmColor = hbmColor; hIcon = CreateIconIndirect(&info); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 57 +++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 48a4652e688..e8158151293 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -82,6 +82,30 @@ static void expect_rawformat(REFGUID expected, GpImage *img, int line, BOOL todo expect_guid(expected, &raw, line, todo); } +static void expect_image_properties(GpImage *image, UINT width, UINT height, int line) +{ + GpStatus stat; + UINT dim; + ImageType type; + PixelFormat format; + + stat = GdipGetImageWidth(image, &dim); + ok_(__FILE__, line)(stat == Ok, "Expected %d, got %d\n", Ok, stat); + ok_(__FILE__, line)(dim == width, "Expected %d, got %d\n", width, dim); + + stat = GdipGetImageHeight(image, &dim); + ok_(__FILE__, line)(stat == Ok, "Expected %d, got %d\n", Ok, stat); + ok_(__FILE__, line)(dim == height, "Expected %d, got %d\n", height, dim); + + stat = GdipGetImageType(image, &type); + ok_(__FILE__, line)(stat == Ok, "Expected %d, got %d\n", Ok, stat); + ok_(__FILE__, line)(type == ImageTypeBitmap, "Expected %d, got %d\n", ImageTypeBitmap, type); + + stat = GdipGetImagePixelFormat(image, &format); + ok_(__FILE__, line)(stat == Ok, "Expected %d, got %d\n", Ok, stat); + ok_(__FILE__, line)(format == PixelFormat32bppARGB, "Expected %d, got %d\n", PixelFormat32bppARGB, format); +} + static BOOL get_encoder_clsid(LPCWSTR mime, GUID *format, CLSID *clsid) { GpStatus status; @@ -1542,9 +1566,6 @@ static void test_fromhicon(void) HICON hIcon; GpStatus stat; GpBitmap *bitmap = NULL; - UINT dim; - ImageType type; - PixelFormat format; /* NULL */ stat = GdipCreateBitmapFromHICON(NULL, NULL); @@ -1570,20 +1591,7 @@ static void test_fromhicon(void) broken(stat == InvalidParameter), /* Win98 */ "Expected Ok, got %.8x\n", stat); if(stat == Ok){ - /* check attributes */ - stat = GdipGetImageHeight((GpImage*)bitmap, &dim); - expect(Ok, stat); - expect(16, dim); - stat = GdipGetImageWidth((GpImage*)bitmap, &dim); - expect(Ok, stat); - expect(16, dim); - stat = GdipGetImageType((GpImage*)bitmap, &type); - expect(Ok, stat); - expect(ImageTypeBitmap, type); - stat = GdipGetImagePixelFormat((GpImage*)bitmap, &format); - expect(Ok, stat); - expect(PixelFormat32bppARGB, format); - /* raw format */ + expect_image_properties((GpImage*)bitmap, 16, 16, __LINE__); expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); GdipDisposeImage((GpImage*)bitmap); } @@ -1605,20 +1613,7 @@ static void test_fromhicon(void) stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); expect(Ok, stat); if(stat == Ok){ - /* check attributes */ - stat = GdipGetImageHeight((GpImage*)bitmap, &dim); - expect(Ok, stat); - expect(16, dim); - stat = GdipGetImageWidth((GpImage*)bitmap, &dim); - expect(Ok, stat); - expect(16, dim); - stat = GdipGetImageType((GpImage*)bitmap, &type); - expect(Ok, stat); - expect(ImageTypeBitmap, type); - stat = GdipGetImagePixelFormat((GpImage*)bitmap, &format); - expect(Ok, stat); - expect(PixelFormat32bppARGB, format); - /* raw format */ + expect_image_properties((GpImage*)bitmap, 16, 16, __LINE__); expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); GdipDisposeImage((GpImage*)bitmap); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index e8158151293..cd9634a6e2a 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1573,18 +1573,17 @@ static void test_fromhicon(void) stat = GdipCreateBitmapFromHICON(NULL, &bitmap); expect(InvalidParameter, stat); - /* color icon 1 bit */ + /* monochrome icon */ hbmMask = CreateBitmap(16, 16, 1, 1, bmp_bits); ok(hbmMask != 0, "CreateBitmap failed\n"); hbmColor = CreateBitmap(16, 16, 1, 1, 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); ok(stat == Ok || @@ -1597,11 +1596,31 @@ static void test_fromhicon(void) } DestroyIcon(hIcon); - /* color icon 8 bpp */ - hbmMask = CreateBitmap(16, 16, 1, 8, bmp_bits); + /* monochrome cursor */ + info.fIcon = FALSE; + info.xHotspot = 8; + info.yHotspot = 8; + info.hbmMask = hbmMask; + info.hbmColor = hbmColor; + hIcon = CreateIconIndirect(&info); + ok(hIcon != 0, "CreateIconIndirect failed\n"); + + stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); + todo_wine + expect(InvalidParameter, stat); + if (stat == Ok) + GdipDisposeImage((GpImage*)bitmap); + DestroyIcon(hIcon); + + DeleteObject(hbmMask); + DeleteObject(hbmColor); + + /* 8 bpp icon */ + hbmMask = CreateBitmap(16, 16, 1, 1, bmp_bits); ok(hbmMask != 0, "CreateBitmap failed\n"); hbmColor = CreateBitmap(16, 16, 1, 8, bmp_bits); ok(hbmColor != 0, "CreateBitmap failed\n"); + info = iconinfo_base; info.hbmMask = hbmMask; info.hbmColor = hbmColor; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/image.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index cd9634a6e2a..b0a17217366 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1612,6 +1612,19 @@ static void test_fromhicon(void) GdipDisposeImage((GpImage*)bitmap); DestroyIcon(hIcon); + /* mask-only icon */ + info = iconinfo_base; + info.hbmMask = hbmMask; + hIcon = CreateIconIndirect(&info); + ok(hIcon != 0, "CreateIconIndirect failed\n"); + + stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); + todo_wine + expect(InvalidParameter, stat); + if (stat == Ok) + GdipDisposeImage((GpImage*)bitmap); + DestroyIcon(hIcon); + DeleteObject(hbmMask); DeleteObject(hbmColor); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/image.c | 84 +++++++++++++------------------------- dlls/gdiplus/tests/image.c | 4 -- 2 files changed, 29 insertions(+), 59 deletions(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 1aa5f59a4e7..7640c934902 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -1635,11 +1635,11 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) TRACE("%p, %p\n", hicon, bitmap); - if(!bitmap || !GetIconInfo(hicon, &iinfo)) + if(!bitmap || !GetIconInfo(hicon, &iinfo) || !iinfo.hbmColor || !iinfo.fIcon) return InvalidParameter; /* get the size of the icon */ - ret = GetObjectA(iinfo.hbmColor ? iinfo.hbmColor : iinfo.hbmMask, sizeof(bm), &bm); + ret = GetObjectA(iinfo.hbmColor, sizeof(bm), &bm); if (ret == 0) { DeleteObject(iinfo.hbmColor); DeleteObject(iinfo.hbmMask); @@ -1647,7 +1647,7 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) } width = bm.bmWidth; - height = iinfo.hbmColor ? abs(bm.bmHeight) : abs(bm.bmHeight) / 2; + height = abs(bm.bmHeight); stride = width * 4; stat = GdipCreateBitmapFromScan0(width, height, stride, PixelFormat32bppARGB, NULL, bitmap); @@ -1672,7 +1672,7 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) bih.biSize = sizeof(bih); bih.biWidth = width; - bih.biHeight = iinfo.hbmColor ? -height: -height * 2; + bih.biHeight = -height; bih.biPlanes = 1; bih.biBitCount = 32; bih.biCompression = BI_RGB; @@ -1683,71 +1683,45 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) bih.biClrImportant = 0; screendc = CreateCompatibleDC(0); - if (iinfo.hbmColor) - { - GetDIBits(screendc, iinfo.hbmColor, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS); + GetDIBits(screendc, iinfo.hbmColor, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS); - if (bm.bmBitsPixel == 32) - { - 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; - } - else has_alpha = FALSE; - } - else + if (bm.bmBitsPixel == 32) { - GetDIBits(screendc, iinfo.hbmMask, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS); 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; } + else has_alpha = FALSE; if (!has_alpha) { - if (iinfo.hbmMask) - { - BYTE *bits = heap_alloc(height * stride); - - /* read alpha data from the mask */ - if (iinfo.hbmColor) - GetDIBits(screendc, iinfo.hbmMask, 0, height, bits, (BITMAPINFO*)&bih, DIB_RGB_COLORS); - else - GetDIBits(screendc, iinfo.hbmMask, height, height, bits, (BITMAPINFO*)&bih, DIB_RGB_COLORS); + BYTE *bits = heap_alloc(height * stride); - 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; - } + /* 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); - heap_free(bits); - } - else + src = (DWORD*)bits; + dst_row = lockeddata.Scan0; + for (y=0; y<height; y++) { - /* set constant alpha of 255 */ - dst_row = lockeddata.Scan0; - for (y=0; y<height; y++) + dst = (DWORD*)dst_row; + for (x=0; x<width; x++) { - 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; } + dst_row += lockeddata.Stride; } + + heap_free(bits); } DeleteDC(screendc); diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index b0a17217366..cbc07b877a9 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1606,7 +1606,6 @@ static void test_fromhicon(void) ok(hIcon != 0, "CreateIconIndirect failed\n"); stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); - todo_wine expect(InvalidParameter, stat); if (stat == Ok) GdipDisposeImage((GpImage*)bitmap); @@ -1619,10 +1618,7 @@ static void test_fromhicon(void) ok(hIcon != 0, "CreateIconIndirect failed\n"); stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); - todo_wine expect(InvalidParameter, stat); - if (stat == Ok) - GdipDisposeImage((GpImage*)bitmap); DestroyIcon(hIcon); DeleteObject(hbmMask); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
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=136599 Your paranoid android. === debian11b (64 bit WoW report) === Report validation errors: ntdll:exception prints too much data (36073 bytes)
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/tests/image.c:
- stat = GdipGetImageType((GpImage*)bitmap, &type); - expect(Ok, stat); - expect(ImageTypeBitmap, type); - stat = GdipGetImagePixelFormat((GpImage*)bitmap, &format); - expect(Ok, stat); - expect(PixelFormat32bppARGB, format); - /* raw format */ + expect_image_properties((GpImage*)bitmap, 16, 16, __LINE__); expect_rawformat(&ImageFormatMemoryBMP, (GpImage*)bitmap, __LINE__, FALSE); GdipDisposeImage((GpImage*)bitmap); } DestroyIcon(hIcon);
- /* color icon 8 bpp */ - hbmMask = CreateBitmap(16, 16, 1, 8, bmp_bits); + /* monochrome cursor */ The comment is not valid here. We are testing here what will happen if `info.fIcon = FALSE`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_43153
On Wed Aug 23 06:36:11 2023 +0000, Bartosz Kosiorek wrote:
The comment is not valid here. We are testing here what will happen if `info.fIcon = FALSE` `info.fIcon = FALSE` makes it a cursor. https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-iconi...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_43196
On Tue Aug 22 22:11:59 2023 +0000, Jeffrey Smith wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/3621/diffs?diff_id=64251&start_sha=7c463ac7a40a6219e4664cfface418bef961e1c4#7c8ca2b5a97a42676b2979286a5fe74c047f321f_85_85) Renamed to `expect_image_properties`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_43197
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621
participants (5)
-
Bartosz Kosiorek (＠gang65) -
Esme Povirk (＠madewokherd)
-
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Marvin