Re: [2/2] windowscodecs: Implement ComponentFactory_CreateBitmapFromMemory.

[Vincent Povirk]

I'm pretty sure CreateBitmapFromMemory copies the data rather than
referencing existing data.

On Thu, Dec 27, 2012 at 3:29 AM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
> ---
>  dlls/windowscodecs/bitmap.c            | 31 +++++++++++++++++++++----------
>  dlls/windowscodecs/imgfactory.c        | 18 +++++++++++-------
>  dlls/windowscodecs/tests/bitmap.c      |  5 -----
>  dlls/windowscodecs/wincodecs_private.h |  1 +
>  4 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/dlls/windowscodecs/bitmap.c b/dlls/windowscodecs/bitmap.c
> index 520fdef..4d48651 100644
> --- a/dlls/windowscodecs/bitmap.c
> +++ b/dlls/windowscodecs/bitmap.c
> @@ -40,6 +40,7 @@ typedef struct BitmapImpl {
>      int palette_set;
>      LONG lock; /* 0 if not locked, -1 if locked for writing, count if locked for reading */
>      BYTE *data;
> +    BOOL owndata;
>      UINT width, height;
>      UINT stride;
>      UINT bpp;
> @@ -260,7 +261,8 @@ static ULONG WINAPI BitmapImpl_Release(IWICBitmap *iface)
>          if (This->palette) IWICPalette_Release(This->palette);
>          This->cs.DebugInfo->Spare[0] = 0;
>          DeleteCriticalSection(&This->cs);
> -        HeapFree(GetProcessHeap(), 0, This->data);
> +        if (This->owndata)
> +            HeapFree(GetProcessHeap(), 0, This->data);
>          HeapFree(GetProcessHeap(), 0, This);
>      }
>
> @@ -447,28 +449,37 @@ static const IWICBitmapVtbl BitmapImpl_Vtbl = {
>  };
>
> HRESULT BitmapImpl_Create(UINT uiWidth, UINT uiHeight,
> +    UINT stride, UINT datasize, BYTE *data,
>      REFWICPixelFormatGUID pixelFormat, WICBitmapCreateCacheOption option,
>      IWICBitmap **ppIBitmap)
>  {
>      HRESULT hr;
>      BitmapImpl *This;
> -    UINT bpp, stride, datasize;
> -    BYTE *data;
> +    UINT bpp;
>
> hr = get_pixelformat_bpp(pixelFormat, &bpp);
>      if (FAILED(hr)) return hr;
>
> -    stride = (((bpp*uiWidth)+31)/32)*4;
> -    datasize = stride * uiHeight;
> +    if (!stride) stride = (((bpp*uiWidth)+31)/32)*4;
> +    if (!datasize) datasize = stride * uiHeight;
> +
> +    if (datasize < stride * uiHeight) return WINCODEC_ERR_INSUFFICIENTBUFFER;
> +    if (stride < (((bpp*uiWidth)+31)/32)*4) return E_INVALIDARG;
>
> This = HeapAlloc(GetProcessHeap(), 0, sizeof(BitmapImpl));
> -    data = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, datasize);
> -    if (!This || !data)
> +    if (!This) return E_OUTOFMEMORY;
> +    if (!data)
>      {
> -        HeapFree(GetProcessHeap(), 0, This);
> -        HeapFree(GetProcessHeap(), 0, data);
> -        return E_OUTOFMEMORY;
> +        data = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, datasize);
> +        if (!data)
> +        {
> +            HeapFree(GetProcessHeap(), 0, This);
> +            return E_OUTOFMEMORY;
> +        }
> +        This->owndata = TRUE;
>      }
> +    else
> +        This->owndata = FALSE;
>
> This->IWICBitmap_iface.lpVtbl = &BitmapImpl_Vtbl;
>      This->ref = 1;
> diff --git a/dlls/windowscodecs/imgfactory.c b/dlls/windowscodecs/imgfactory.c
> index 54b390b..1d05ef5 100644
> --- a/dlls/windowscodecs/imgfactory.c
> +++ b/dlls/windowscodecs/imgfactory.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright 2009 Vincent Povirk for CodeWeavers
> + * Copyright 2012 Dmitry Timoshkov
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -459,7 +460,7 @@ static HRESULT WINAPI ComponentFactory_CreateBitmap(IWICComponentFactory *iface,
>  {
>      TRACE("(%p,%u,%u,%s,%u,%p)\n", iface, uiWidth, uiHeight,
>          debugstr_guid(pixelFormat), option, ppIBitmap);
> -    return BitmapImpl_Create(uiWidth, uiHeight, pixelFormat, option, ppIBitmap);
> +    return BitmapImpl_Create(uiWidth, uiHeight, 0, 0, NULL, pixelFormat, option, ppIBitmap);
>  }
>
> static HRESULT WINAPI ComponentFactory_CreateBitmapFromSource(IWICComponentFactory *iface,
> @@ -506,7 +507,7 @@ static HRESULT WINAPI ComponentFactory_CreateBitmapFromSource(IWICComponentFacto
>      }
>
> if (SUCCEEDED(hr))
> -        hr = BitmapImpl_Create(width, height, &pixelformat, option, &result);
> +        hr = BitmapImpl_Create(width, height, 0, 0, NULL, &pixelformat, option, &result);
>
> if (SUCCEEDED(hr))
>      {
> @@ -576,12 +577,15 @@ static HRESULT WINAPI ComponentFactory_CreateBitmapFromSourceRect(IWICComponentF
>  }
>
> static HRESULT WINAPI ComponentFactory_CreateBitmapFromMemory(IWICComponentFactory *iface,
> -    UINT uiWidth, UINT uiHeight, REFWICPixelFormatGUID pixelFormat, UINT cbStride,
> -    UINT cbBufferSize, BYTE *pbBuffer, IWICBitmap **ppIBitmap)
> +    UINT width, UINT height, REFWICPixelFormatGUID format, UINT stride,
> +    UINT size, BYTE *buffer, IWICBitmap **bitmap)
>  {
> -    FIXME("(%p,%u,%u,%s,%u,%u,%p,%p): stub\n", iface, uiWidth, uiHeight,
> -        debugstr_guid(pixelFormat), cbStride, cbBufferSize, pbBuffer, ppIBitmap);
> -    return E_NOTIMPL;
> +    TRACE("(%p,%u,%u,%s,%u,%u,%p,%p\n", iface, width, height,
> +        debugstr_guid(format), stride, size, buffer, bitmap);
> +
> +    if (!stride || !size || !buffer || !bitmap) return E_INVALIDARG;
> +
> +    return BitmapImpl_Create(width, height, stride, size, buffer, format, WICBitmapCacheOnLoad, bitmap);
>  }
>
> static HRESULT WINAPI ComponentFactory_CreateBitmapFromHBITMAP(IWICComponentFactory *iface,
> diff --git a/dlls/windowscodecs/tests/bitmap.c b/dlls/windowscodecs/tests/bitmap.c
> index cec714b..3cdbffd 100644
> --- a/dlls/windowscodecs/tests/bitmap.c
> +++ b/dlls/windowscodecs/tests/bitmap.c
> @@ -436,27 +436,22 @@ static void test_CreateBitmapFromMemory(void)
>
> hr = IWICImagingFactory_CreateBitmapFromMemory(factory, 3, 3, &GUID_WICPixelFormat24bppBGR,
>                                                     0, 0, NULL, &bitmap);
> -todo_wine
>      ok(hr == E_INVALIDARG, "expected E_INVALIDARG, got %#x\n", hr);
>
> hr = IWICImagingFactory_CreateBitmapFromMemory(factory, 3, 3, &GUID_WICPixelFormat24bppBGR,
>                                                     0, sizeof(data3x3), data3x3, &bitmap);
> -todo_wine
>      ok(hr == E_INVALIDARG, "expected E_INVALIDARG, got %#x\n", hr);
>
> hr = IWICImagingFactory_CreateBitmapFromMemory(factory, 3, 3, &GUID_WICPixelFormat24bppBGR,
>                                                     6, sizeof(data3x3), data3x3, &bitmap);
> -todo_wine
>      ok(hr == E_INVALIDARG, "expected E_INVALIDARG, got %#x\n", hr);
>
> hr = IWICImagingFactory_CreateBitmapFromMemory(factory, 3, 3, &GUID_WICPixelFormat24bppBGR,
>                                                     12, sizeof(data3x3), data3x3, &bitmap);
> -todo_wine
>      ok(hr == WINCODEC_ERR_INSUFFICIENTBUFFER, "expected WINCODEC_ERR_INSUFFICIENTBUFFER, got %#x\n", hr);
>
> hr = IWICImagingFactory_CreateBitmapFromMemory(factory, 3, 3, &GUID_WICPixelFormat24bppBGR,
>                                                     9, sizeof(data3x3) - 1, data3x3, &bitmap);
> -todo_wine
>      ok(hr == WINCODEC_ERR_INSUFFICIENTBUFFER, "expected WINCODEC_ERR_INSUFFICIENTBUFFER, got %#x\n", hr);
>
> hr = IWICImagingFactory_CreateBitmapFromMemory(factory, 3, 3, &GUID_WICPixelFormat24bppBGR,
> diff --git a/dlls/windowscodecs/wincodecs_private.h b/dlls/windowscodecs/wincodecs_private.h
> index 6857176..508bb2c 100644
> --- a/dlls/windowscodecs/wincodecs_private.h
> +++ b/dlls/windowscodecs/wincodecs_private.h
> @@ -45,6 +45,7 @@ extern HRESULT IcnsEncoder_CreateInstance(IUnknown *pUnkOuter, REFIID iid, void*
>  extern HRESULT TgaDecoder_CreateInstance(IUnknown *pUnkOuter, REFIID iid, void** ppv) DECLSPEC_HIDDEN;
>
> extern HRESULT BitmapImpl_Create(UINT uiWidth, UINT uiHeight,
> +    UINT stride, UINT datasize, BYTE *data,
>      REFWICPixelFormatGUID pixelFormat, WICBitmapCreateCacheOption option,
>      IWICBitmap **ppIBitmap) DECLSPEC_HIDDEN;
>  extern HRESULT BitmapScaler_Create(IWICBitmapScaler **scaler) DECLSPEC_HIDDEN;
> --
> 1.8.0.2

[2/2] windowscodecs: Implement ComponentFactory_CreateBitmapFromMemory.

[Dmitry Timoshkov]

Vincent Povirk <madewokherd(a)gmail.com> wrote:
> I'm pretty sure CreateBitmapFromMemory copies the data rather than
> referencing existing data.

How do you know that? In any case adding a test case and support for that
behaviour should be pretty trivial once this patch is accepted.

-- 
Dmitry.

[2/2] windowscodecs: Implement ComponentFactory_CreateBitmapFromMemory.

[Vincent Povirk]

> > I'm pretty sure CreateBitmapFromMemory copies the data rather than
> > referencing existing data.
>
> How do you know that? In any case adding a test case and support for that
> behaviour should be pretty trivial once this patch is accepted.

I believe it was this stack overflow question that first tipped me
off: http://stackoverflow.com/questions/11263099/render-direct2d-into-existing-me...

There's also a comment on the documentation that suggests it does a
copy: http://msdn.microsoft.com/en-us/library/ee690291%28v=vs.85%29.aspx

Note that this was all before I did any real work with IWICBitmap.

I think I tested this when I first implemented IWICBimap, to decide
whether the ability to reference existing data would be needed at all,
but if so I no longer have that test case. It should be trivial to add
to your test.

So, this is trivial to test and would mean that the features your
patch would add are no longer needed (except possibly the ability to
supply a stride for the new bitmap - but I would test that as well, as
it seems very strange to me that WIC would copy the bits without
removing the gaps). Potentially, your whole patch might have to be
reverted in favor of copying image data in CreateBitmapFromMemory. I
think it's better to skip adding new functionality than add it and
remove it later.

[2/2] windowscodecs: Implement ComponentFactory_CreateBitmapFromMemory.

[Vincent Povirk]

Looks like I won't be around when this finishes, but I sent a test to
testbot: https://testbot.winehq.org/JobDetails.pl?Key=23638

This should succeed if your implementation is correct (data is
referenced, not copied).