[PATCH 0/3] MR3621: GdipCreateBitmapFromICON fixes part 1/2

[PATCH 1/3] gdiplus/tests: Simplify ICONINFO initialization.

[Jeff Smith]

From: Jeff Smith

---
 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);

[PATCH 2/3] gdiplus/tests: Add test for mask-only icon with GdipCreateBitmapFromHICON.

[Jeff Smith]

From: Jeff Smith

---
 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");

[PATCH 3/3] gdiplus: Fix GdipCreateBitmapFromICON return status with mask-only icon.

[Jeff Smith]

From: Jeff Smith

---
 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);

[Jeffrey Smith (＠whydoubt)]

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.

[Bartosz Kosiorek (＠gang65)]

Great improvement @whydoubt 
Out of curiosity: How did you find such issue?
Is there some application which is not working correctly with gdiplus?

[Jeffrey Smith (＠whydoubt)]

@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.

[Bartosz Kosiorek (＠gang65)]

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 
```suggestion:-0+0
            for (x=0; x<width; x++)
```

[Bartosz Kosiorek (＠gang65)]

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.

[Bartosz Kosiorek (＠gang65)]

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.

[Bartosz Kosiorek (＠gang65)]

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 */
```

[Jeffrey Smith (＠whydoubt)]

On Mon Aug 21 18:16:35 2023 +0000, Bartosz Kosiorek wrote:
> It should be
>             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.

[Jeffrey Smith (＠whydoubt)]

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.

[Jeffrey Smith (＠whydoubt)]

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).

[Jeffrey Smith (＠whydoubt)]

On Mon Aug 21 18:13:05 2023 +0000, Bartosz Kosiorek wrote:
> I would align description with other test cases:
>     /* color icon 1 bit - color and mask */

I've been thinking on that, and I may tweak the new comments instead.

[Bartosz Kosiorek (＠gang65)]

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

[Bartosz Kosiorek (＠gang65)]

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`

[Bartosz Kosiorek (＠gang65)]

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)) {
```

[Jeffrey Smith (＠whydoubt)]

On Mon Aug 21 18:54:47 2023 +0000, Bartosz Kosiorek wrote:
> Generally, we should checkout the return value of `GetDIBits`, like:
>     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.

[Jeffrey Smith (＠whydoubt)]

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.

[Jeffrey Smith (＠whydoubt)]

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.

[Esme Povirk (＠madewokherd)]

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?

[Jeffrey Smith (＠whydoubt)]

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.

[Bartosz Kosiorek (＠gang65)]

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

[Bartosz Kosiorek (＠gang65)]

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.

[Jeffrey Smith (＠whydoubt)]

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 ...".