Re: windowscodecs: Implement CreateBitmapFromHBITMAP.

[Vincent Povirk]

Maybe the format for 32-bit bitmaps should be determined by the alpha
channel options?

On Monday, September 16, 2013, Dmitry Timoshkov  wrote:
> Based on a gdiplus implementation.
>
> dlls/windowscodecs/imgfactory.c   | 149

+++++++++++++++++++++++++++++++++++++-
> dlls/windowscodecs/tests/bitmap.c |  14 ++--
>  2 files changed, 154 insertions(+), 9 deletions(-)
>
> diff --git a/dlls/windowscodecs/imgfactory.c

b/dlls/windowscodecs/imgfactory.c
> index e5f2125..bb4cf67 100644
> --- a/dlls/windowscodecs/imgfactory.c
> +++ b/dlls/windowscodecs/imgfactory.c
> @@ -603,12 +603,153 @@ static HRESULT WINAPI

ComponentFactory_CreateBitmapFromMemory(IWICComponentFacto
>  return BitmapImpl_Create(width, height, stride, size, buffer,

format, WICBitmapCacheOnLoad, bitmap);
> }
>
> +static BOOL get_16bpp_format(HBITMAP hbm, WICPixelFormatGUID *format)
> +{
> - BOOL ret = TRUE;
> - BITMAPV4HEADER bmh;
> - HDC hdc;
> -
> - hdc = CreateCompatibleDC(0);
> -
> - memset(&bmh, 0, sizeof(bmh));
> - bmh.bV4Size = sizeof(bmh);
> - bmh.bV4Width = 1;
> - bmh.bV4Height = 1;
> - bmh.bV4V4Compression = BI_BITFIELDS;
> - bmh.bV4BitCount = 16;
> -
> - GetDIBits(hdc, hbm, 0, 0, NULL, (BITMAPINFO *)&ji, DIB_RGB_COLORS);
> -
> - if (bmh.bV4RedMask == 0x7c00 &&
> -
>    bmh.bV4GreenMask == 0x3e0 &&
> -
>    bmh.bV4BlueMask == 0x1f)
> - {
> -
>    *format = GUID_WICPixelFormat16bppBGR555;
> - }
> - else if (bmh.bV4RedMask == 0xf800 &&
> -
>    bmh.bV4GreenMask == 0x7e0 &&
> -
>    bmh.bV4BlueMask == 0x1f)
> - {
> -
>    *format = GUID_WICPixelFormat16bppBGR565;
> - }
> - else
> - {
> -
>    FIXME("unrecognized bitfields %x,%x,%x\n", bmh.bV4RedMask,
> -
>        bmh.bV4GreenMask, bmh.bV4BlueMask);
> -
>    ret = FALSE;
> - }
> -
> - DeleteDC(hdc);
> - return ret;
>
> +}
> -
>
> static HRESULT WINAPI

ComponentFactory_CreateBitmapFromHBITMAP(IWICComponentFactory *iface,
> - HBITMAP hBitmap, HPALETTE hPalette, WICBitmapAlphaChannelOption

options,
> - IWICBitmap **ppIBitmap)
> - HBITMAP hbm, HPALETTE hpal, WICBitmapAlphaChannelOption option,

IWICBitmap **bitmap)
> {
> - FIXME("(%p,%p,%p,%u,%p): stub\n", iface, hBitmap, hPalette, options,

ppIBitmap);
> - return E_NOTIMPL;
> - BITMAP bm;
> - HRESULT hr;
> - WICPixelFormatGUID format;
> - IWICBitmapLock *lock;
> - UINT size, num_palette_entries = 0;
> - PALETTEENTRY entry[256];
> -
> - TRACE("(%p,%p,%p,%u,%p)\n", iface, hbm, hpal, option, bitmap);
> -
> - if (!bitmap) return E_INVALIDARG;
> -
> - if (GetObjectW(hbm, sizeof(bm), &bm) != sizeof(bm))
> -
>    return WINCODEC_ERR_WIN32ERROR;
> -
> - if (hpal)
> - {
> -
>    num_palette_entries = GetPaletteEntries(hpal, 0, 256, entry);
> -
>    if (!num_palette_entries)
> -
>        return WINCODEC_ERR_WIN32ERROR;
> - }
> -
> - /* TODO: Figure out the correct format for 16, 32, 64 bpp */
> - switch(bm.bmBitsPixel)
> - {
> - case 1:
> -
>    format = GUID_WICPixelFormat1bppIndexed;
> -
>    break;
> - case 4:
> -
>    format = GUID_WICPixelFormat4bppIndexed;
> -
>    break;
> - case 8:
> -
>    format = GUID_WICPixelFormat8bppIndexed;
> -
>    break;
> - case 16:
> -
>    if (!get_16bpp_format(hbm, &format))
> -
>        return E_INVALIDARG;
> -
>    break;
> - case 24:
> -
>    format = GUID_WICPixelFormat24bppBGR;
> -
>    break;
> - case 32:
> -
>    format = GUID_WICPixelFormat32bppBGR;
> -
>    break;
> - case 48:
> -
>    format = GUID_WICPixelFormat48bppRGB;
> -
>    break;
> - default:
> -
>    FIXME("unsupported %d bpp\n", bm.bmBitsPixel);
> -
>    return E_INVALIDARG;
> - }
> -
> - hr = BitmapImpl_Create(bm.bmWidth, bm.bmHeight, bm.bmWidthBytes, 0,

NULL, &format, option, bitmap);
> - if (hr != S_OK) return hr;
> -
> - hr = IWICBitmap_Lock(*bitmap, NULL, WICBitmapLockWrite, &lock);
> - if (hr == S_OK)
> - {
> -
>    BYTE *buffer;
> -
>    HDC hdc;
> -
>    char bmibuf[FIELD_OFFSET(BITMAPINFO, bmiColors[256])];
> -
>    BITMAPINFO *bmi = (BITMAPINFO *)bmibuf;
> -
> -
>    IWICBitmapLock_GetDataPointer(lock, &size, &buffer);
> -
> -
>    hdc = CreateCompatibleDC(0);
> -
> -
>    bmi->bmiHeader.biSize = sizeof(BITMAPINFOHEADER);
> -
>    bmi->bmiHeader.biBitCount = 0;
> -
>    GetDIBits(hdc, hbm, 0, 0, NULL, bmi, DIB_RGB_COLORS);
> -
>    bmi->bmiHeader.biHeight = -bm.bmHeight;
> -
>    GetDIBits(hdc, hbm, 0, bm.bmHeight, buffer, bmi, DIB_RGB_COLORS);
> -
> -
>    DeleteDC(hdc);
> -
>    IWICBitmapLock_Release(lock);
> -
> -
>    if (num_palette_entries)
> -
>    {
> -
>        IWICPalette *palette;
> -
>        WICColor colors[256];
> -
>        UINT i;
> -
> -
>        hr = PaletteImpl_Create(&palette);
> -
>        if (hr == S_OK)
> -
>        {
> -
>            for (i = 0; i < num_palette_entries; i++)
> -
>                colors[i] = 0xff000000 | entry[i].peRed << 16 |
> -
>                            entry[i].peGreen << 8 | entry[i].peBlue;
> -
> -
>            hr = IWICPalette_InitializeCustom(palette, colors,

num_palette_entries);
> -
>            if (hr == S_OK)
> -
>                hr = IWICBitmap_SetPalette(*bitmap, palette);
> -
> -
>            IWICPalette_Release(palette);
> -
>        }
> -
>    }
> - }
> -
> - if (hr != S_OK)
> - {
> -
>    IWICBitmap_Release(*bitmap);
> -
>    *bitmap = NULL;
> - }
> -
> - return hr;
>
> }
>
> static HRESULT WINAPI

ComponentFactory_CreateBitmapFromHICON(IWICComponentFactory *iface,
> diff --git a/dlls/windowscodecs/tests/bitmap.c

b/dlls/windowscodecs/tests/bitmap.c
> index 8795048..3c79ce8 100644
> --- a/dlls/windowscodecs/tests/bitmap.c
> +++ b/dlls/windowscodecs/tests/bitmap.c
> @@ -727,17 +727,13 @@ static void test_CreateBitmapFromHBITMAP(void)
>      ok(hbmp != 0, "failed to create bitmap\n");
>  hr = IWICImagingFactory_CreateBitmapFromHBITMAP(factory, 0, 0,

WICBitmapIgnoreAlpha, &bitmap);
> -todo_wine
>      ok(hr == WINCODEC_ERR_WIN32ERROR || hr == 0x88980003 /*XP*/,

"expected WINCODEC_ERR_WIN32ERROR, got %#x\n", hr);
>  hr = IWICImagingFactory_CreateBitmapFromHBITMAP(factory, hbmp, 0,

WICBitmapIgnoreAlpha, NULL);
> -todo_wine
>      ok(hr == E_INVALIDARG, "expected E_INVALIDARG, got %#x\n", hr);
>  hr = IWICImagingFactory_CreateBitmapFromHBITMAP(factory, hbmp, 0,

WICBitmapIgnoreAlpha, &bitmap);
> -todo_wine
>      ok(hr == S_OK, "CreateBitmapFromHBITMAP error %#x\n", hr);
> -
>
> if (hr != S_OK) return;
>
> IWICBitmap_GetPixelFormat(bitmap, &format);
> ok(IsEqualGUID(&format, &GUID_WICPixelFormat8bppIndexed),
>
> @@ -770,6 +766,7 @@ todo_wine
>      ok(hr == S_OK, "CreateBitmapFromHBITMAP error %#x\n", hr);
>  IWICBitmap_GetPixelFormat(bitmap, &format);
>
> +todo_wine
>      ok(IsEqualGUID(&format, &GUID_WICPixelFormat4bppIndexed),
>         "unexpected pixel format %s\n", debugstr_guid(&format));
>
> @@ -789,6 +786,7 @@ todo_wine
>  hr = IWICPalette_GetColorCount(palette, &count);
>  ok(hr == S_OK, "GetColorCount error %#x\n", hr);
>
> +todo_wine
>      ok(count == 16, "expected 16, got %u\n", count);
>  IWICPalette_Release(palette);
>
> @@ -837,7 +835,13 @@ todo_wine
>      hr = IWICBitmap_CopyPixels(bitmap, NULL, 4, sizeof(data), data);
>      ok(hr == S_OK, "IWICBitmap_CopyPixels error %#x\n", hr);
>      for (i = 0; i < sizeof(data); i++)
> -
>    ok(data[i] == data_8bpp_pal_wic[i], "%u: expected %#x, got

%#x\n", i, data_8bpp_pal_wic[i], data[i]);
> - {
> -
>    if (data[i] != data_8bpp_pal_wic[i])
>
> +todo_wine
> -
>        ok(data[i] == data_8bpp_pal_wic[i], "%u: expected %#x, got

%#x\n", i, data_8bpp_pal_wic[i], data[i]);
> -
>    else
> -
>        ok(data[i] == data_8bpp_pal_wic[i], "%u: expected %#x, got

%#x\n", i, data_8bpp_pal_wic[i], data[i]);
> -
>
> }
>
> IWICBitmap_Release(bitmap);
> DeleteObject(hbmp);
>
> --
> 1.8.3.4

windowscodecs: Implement CreateBitmapFromHBITMAP.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> Maybe the format for 32-bit bitmaps should be determined by the alpha
> channel options?

Probably, but as the patch comment says the code is based on gdiplus
implementation, and there is no such an option flag in the corresponding
gdiplus API params. I'd suggest to accept the patch as as, and make further
improvements based on it.

windowscodecs: Implement CreateBitmapFromHBITMAP.

[Vincent Povirk]

> Probably, but as the patch comment says the code is based on gdiplus
> implementation, and there is no such an option flag in the corresponding
> gdiplus API params. I'd suggest to accept the patch as as, and make further
> improvements based on it.

Fair enough, the patch looks good otherwise.