[PATCH v2 0/5] MR3621: GdipCreateBitmapFromICON fixes part 1/2
-- v2: 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 attributes. gdiplus/tests: Simplify ICONINFO initialization. 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..347634856e0 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_attributes(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_attributes((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_attributes((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 347634856e0..f288e0652ed 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); + + /* 8bpp 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 f288e0652ed..9867ab22632 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 9867ab22632..64450ab4efe 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
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/image.c:
expect_guid(expected, &raw, line, todo); }
+static void expect_image_attributes(GpImage *image, UINT width, UINT height, int line)
This name might be confusing since there's an unrelated object type called ImageAttributes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_43093
On Tue Aug 22 20:59:24 2023 +0000, Esme Povirk wrote:
This name might be confusing since there's an unrelated object type called ImageAttributes. Yeah, I guess that could cause confusion. I will look for a better name.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_43095
participants (3)
-
Esme Povirk (＠madewokherd) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)