[PATCH 0/3] MR3621: GdipCreateBitmapFromICON fixes part 1/2
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 | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 48a4652e688..2523ee61f71 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1562,8 +1562,6 @@ static void test_fromhicon(void) info.hbmColor = hbmColor; hIcon = CreateIconIndirect(&info); ok(hIcon != 0, "CreateIconIndirect failed\n"); - DeleteObject(hbmMask); - DeleteObject(hbmColor); stat = GdipCreateBitmapFromHICON(hIcon, &bitmap); ok(stat == Ok || @@ -1589,6 +1587,28 @@ static void test_fromhicon(void) } DestroyIcon(hIcon); + /* color icon 1 bit - color-only */ + info = iconinfo_base; + info.hbmColor = hbmColor; + hIcon = CreateIconIndirect(&info); + ok(hIcon == 0, "CreateIconIndirect expected to fail\n"); + + /* color icon 1 bit - mask-only */ + 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); + /* color icon 8 bpp */ hbmMask = CreateBitmap(16, 16, 1, 8, bmp_bits); ok(hbmMask != 0, "CreateBitmap failed\n"); -- 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 | 3 -- 2 files changed, 29 insertions(+), 58 deletions(-) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 3dfc2fec763..451e7931445 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) 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 (x=0; x<width && !has_alpha; x++) - for (y=0; y<height && !has_alpha; y++) - 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 (x=0; x<width && !has_alpha; x++) + for (y=0; y<height && !has_alpha; y++) + if ((*src++ & 0xff000000) != 0) + has_alpha = TRUE; } + else has_alpha = FALSE; if (!has_alpha) { - if (iinfo.hbmMask) - { - BYTE *bits = heap_alloc(height * stride); + 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); - - src = (DWORD*)bits; - dst_row = lockeddata.Scan0; - for (y=0; y<height; y++) - { - dst = (DWORD*)dst_row; - for (x=0; x<height; x++) - { - DWORD src_value = *src++; - if (src_value) - *dst++ = 0; - else - *dst++ |= 0xff000000; - } - dst_row += lockeddata.Stride; - } + /* read alpha data from the mask */ + 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<height; x++) { - dst = (DWORD*)dst_row; - for (x=0; x<height; 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 2523ee61f71..392282720a8 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -1600,10 +1600,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
Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/image.c:
{ static const BYTE bmp_bits[1024]; HBITMAP hbmMask, hbmColor; - ICONINFO info; + ICONINFO info, iconinfo_base = {TRUE, 0, 0, 0, 0}; Note: user32 cursoricon tests show that any provided hotspot is ignored when creating icons with CreateIconIndirect.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42804
Great improvement @whydoubt Out of curiosity: How did you find such issue? Is there some application which is not working correctly with gdiplus? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42881
@gang65 I ran across a tiny 'old' MR in gitlab that made me realize GdipCreatBitmapFromHICON could use some better tests. In creating those, I realized the wine implementation contained a fair bit of unreachable code. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42884
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/image.c:
- } + /* read alpha data from the mask */ + 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<height; x++) I think it should be
for (x=0; x<width; x++)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42885
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/tests/image.c:
/* color icon 1 bit */ hbmMask = CreateBitmap(16, 16, 1, 1, bmp_bits); ok(hbmMask != 0, "CreateBitmap failed\n"); hbmColor = CreateBitmap(16, 16, 1, 1, bmp_bits);
`bmp_bits` is never initialized, which usually means it is filled with zeros. I would recommend to use different values for mask and for Color, to have more real life scenario. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42888
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/tests/image.c:
info.hbmColor = hbmColor; hIcon = CreateIconIndirect(&info); ok(hIcon != 0, "CreateIconIndirect failed\n"); - DeleteObject(hbmMask); - DeleteObject(hbmColor);
stat = GdipCreateBitmapFromHICON(hIcon, &bitmap);
It would be perfect to verify, what kind of image is produced. Currently we are not checking the content of the image. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42889
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/tests/image.c:
stat = GdipCreateBitmapFromHICON(NULL, &bitmap); expect(InvalidParameter, stat);
/* color icon 1 bit */
I would align description with other test cases: ```suggestion:-0+0 /* color icon 1 bit - color and mask */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42890
On Mon Aug 21 18:16:35 2023 +0000, Bartosz Kosiorek wrote:
It should be ```suggestion:-0+0 for (x=0; x<width; x++) ``` It would be great to cover it with test case. currenlty icon is filled with zeros, that's why it is not verify this case. That will all be in in part 2. I didn't want to pack too much into this MR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42891
On Mon Aug 21 18:09:11 2023 +0000, Bartosz Kosiorek wrote:
`bmp_bits` is never initialized, which usually means it is filled with zeros. I would recommend to use different values for mask and for Color, to have more real life scenario. In part 2, I do initialize bmp_bits.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42892
On Mon Aug 21 18:21:42 2023 +0000, Bartosz Kosiorek wrote:
It would be perfect to verify, what kind of image is produced. Currently we are not checking the content of the image. It will allow us to compare image produced by native gdiplus and from Wine. Part 2 will introduce checks for the bitmap data produced (though not here as no bitmap is created).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42893
On Mon Aug 21 18:13:05 2023 +0000, Bartosz Kosiorek wrote:
I would align description with other test cases: ```suggestion:-0+0 /* color icon 1 bit - color and mask */ ``` I've been thinking on that, and I may tweak the new comments instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42894
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/image.c:
TRACE("%p, %p\n", hicon, bitmap);
- if(!bitmap || !GetIconInfo(hicon, &iinfo)) + if(!bitmap || !GetIconInfo(hicon, &iinfo) || !iinfo.hbmColor)
We should also check if `iinfo.fIcon` is true and cover it with test case -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42895
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/tests/image.c:
ok(hbmMask != 0, "CreateBitmap failed\n"); hbmColor = CreateBitmap(16, 16, 1, 1, bmp_bits); ok(hbmColor != 0, "CreateBitmap failed\n"); - info.fIcon = TRUE;
We don't have test case for `info.fIcon = FALSE` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42896
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/image.c:
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);
Generally, we should checkout the return value of `GetDIBits`, like: ```suggestion:-0+0 if (GetDIBits(screendc, iinfo.hbmColor, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS)) { ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42897
On Mon Aug 21 18:54:47 2023 +0000, Bartosz Kosiorek wrote:
Generally, we should checkout the return value of `GetDIBits`, like: ```suggestion:-0+0 if (GetDIBits(screendc, iinfo.hbmColor, 0, height, lockeddata.Scan0, (BITMAPINFO*)&bih, DIB_RGB_COLORS)) { ``` and exit properly if it is fails (`DeleteDC`). It does seem like a good idea to check the return value and respond appropriately. I think I can add such a check in a cleaner way in part 2, than in part 1.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42898
On Mon Aug 21 18:28:13 2023 +0000, Bartosz Kosiorek wrote:
We don't have test case for `info.fIcon = FALSE` It appears that an HICON created with `info.fIcon = FALSE` (and is thus actually a cursor) works fine with GdipCreateBitmapFromHICON. I will get that test added into this MR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42899
On Mon Aug 21 18:30:35 2023 +0000, Bartosz Kosiorek wrote:
We should also check if `iinfo.fIcon` is true and cover it with test case It appears that if `iinfo.fIcon` is true, this function still work fine, so no change is needed here, but I will get tests updated.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42900
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/image.c:
} DestroyIcon(hIcon);
+ /* color icon 1 bit - color-only */ + info = iconinfo_base; + info.hbmColor = hbmColor; + hIcon = CreateIconIndirect(&info); + ok(hIcon == 0, "CreateIconIndirect expected to fail\n");
This isn't really testing gdiplus, is it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42905
On Mon Aug 21 20:35:42 2023 +0000, Esme Povirk wrote:
This isn't really testing gdiplus, is it? No, but one of the code paths that I am removing appears to assume this case is possible, so I put this here to disabuse anyone of the notion that it was possible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42906
On Mon Aug 21 19:57:55 2023 +0000, Jeffrey Smith wrote:
It does seem like a good idea to check the return value and respond appropriately. I think I can add such a check in a cleaner way in part 2, than in part 1. yes, let's do that in part 2
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42927
On Mon Aug 21 20:49:16 2023 +0000, Jeffrey Smith wrote:
No, but one of the code paths that I am removing appears to assume this case is possible, so I put this here to disabuse anyone of the notion that it was possible. This test was already implemented here: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/user32/tests/cursoric...
I think we could leave comment instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42928
On Mon Aug 21 21:23:02 2023 +0000, Bartosz Kosiorek wrote:
This test was already implemented here: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/user32/tests/cursoric... I think we could leave comment instead. I suppose a comment would be sufficient. I was just anticipating a "why are you not testing for ...".
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3621#note_42930
participants (4)
-
Bartosz Kosiorek (＠gang65) -
Esme Povirk (＠madewokherd)
-
Jeff Smith
-
Jeffrey Smith (＠whydoubt)