[PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Henri Verbeet]

On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta at gmail.com> wrote:
> v3: Add bitmap verification, use InitFromFile and other minor improvements.
>
Implementing InitFromFile() should be a separate patch from
implementing LoadTexture().

>      if (FAILED(hr = d3drm_texture_create(&object)))
>          return hr;
>
> +    if (FAILED(hr = IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface, filename)))
> +    {
> +        d3drm_texture_destroy(object);
> +        return hr;
> +    }
> +
> +    object->d3drm = iface;
> +    IDirect3DRM_AddRef(iface);
It would be good to back this up with tests. It would seem more
natural to pass the IDirect3DRM interface to d3drm_texture_create().
If that's how it's supposed to work, you can make that change in a
separate patch.

> @@ -39,6 +40,9 @@ struct d3drm_texture
>      IDirect3DRMTexture IDirect3DRMTexture_iface;
>      IDirect3DRMTexture2 IDirect3DRMTexture2_iface;
>      IDirect3DRMTexture3 IDirect3DRMTexture3_iface;
> +    IDirect3DRM *d3drm;
> +    D3DRMIMAGE *image;
> +    BOOL internal_image_created;
You don't really need internal_image_created, you can probably just
add a destroy callback.

> +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path,
> +            BOOL load_upside_down) DECLSPEC_HIDDEN;
> +
> +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down) DECLSPEC_HIDDEN;
These are internal helper functions now.

> @@ -4109,36 +4111,94 @@ static void test_load_texture(void)
>
>          hr = IDirect3DRM_LoadTexture(d3drm1, filename, &texture1);
>          ok(SUCCEEDED(hr), "Test %u: Failed to load texture, hr %#x.\n", i, hr);
> +        ref2 = get_refcount((IUnknown *)d3drm1);
> +        ok(ref2 > ref1, "Test %u: expected ref2 > ref1, got ref1 = %u, ref2 = %u.\n", i, ref1, ref2);
> +
> +        hr = IDirect3DRMTexture_InitFromFile(texture1, filename);
> +        ok(hr == D3DRMERR_BADOBJECT, "Test %u: Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", i, hr);
> +        /* It's odd, but InitFromFile seems to AddRef IDirect3DRM even if it fails. */
So a test for the refcounts would be appropriate here.

> +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path, BOOL load_upside_down)
> +{
This is an internal helper function now and should be static. You
never use "d3drm".

> +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down)
> +{
> +    D3DRMPALETTEENTRY *colors;
> +    D3DRMIMAGE *image;
> +    BOOL black_used = FALSE;
> +    LONG w = info->bmiHeader.biWidth;
> +    LONG h = abs(info->bmiHeader.biHeight);
> +    int i, j, k;
> +    unsigned int idx, buffer1_idx;
> +    unsigned char *buffer1;
> +    int bpp = info->bmiHeader.biBitCount == 24 ? 32 : info->bmiHeader.biBitCount;
> +    int bpl = palette ? w : ((w + 3) & ~3) * bpp / 8;
> +    int num_colors = 0;
There's not reason to use signed types for any of these.

> +    struct colors_24bpp
> +    {
> +        BYTE red;
> +        BYTE green;
> +        BYTE blue;
> +    } *color;
> +    color = (struct colors_24bpp *)buffer;
> +
> +    if (bpl * h > UINT_MAX)
> +    {
> +        return NULL;
> +    }
This doesn't work. If "bpl * h" would be larger than UINT_MAX it would
overflow before the comparison. In fact, strictly speaking things
become implementation defined above INT_MAX already, since you're
using signed types for "bpl" and "h". What you'd typically do is
something like "if (h > UINT_MAX / bpl)". Note that the calculation of
"bpl" itself can overflow as well. So you'd need something like "if
(align(w, 4) > UINT_MAX / (bpp / 8))" there as well.

> +    if (!(colors = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 256 * sizeof(*colors))))
> +    {
> +        WARN("Not enough memory to allocate palette, returning NULL.\n");
> +        return NULL;
> +    }
> +
> +
> +    if (!(image = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*image))))
> +    {
> +       WARN("Not enough memory to allocate image struct, returning NULL.\n");
> +        return NULL;
> +    }
You probably don't need HEAP_ZERO_MEMORY for either of these. You
certainly don't for "colors", and you should probably just initialise
the few fields in "image" that you don't already. You also leak
"colors" if the allocation for "image" fails. The indentation for the
WARN is wrong.

> +    if (!(image->buffer1 = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, bpl * h)))
> +    {
> +        WARN("Not enough memory to allocate image buffer, returning NULL.\n");
> +        return NULL;
> +    }
You don't need HEAP_ZERO_MEMORY here if you're going to memset() it
again below. You leak "colors" and "image" on failure.

> +    buffer1 = image->buffer1;
> +    memset(buffer1, 255, sizeof(unsigned char) * bpl * h);
I think 0xff is clearer than 255. The "sizeof(unsigned char)" doesn't add much.

> +            if (!(image->palette = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, num_colors * sizeof(*image->palette))))
> +            {
> +                WARN("Not enough memory to allocate image palette, returning NULL.\n");
> +                return NULL;
> +            }
Like above, you use HEAP_ZERO_MEMORY here, but immediately memcpy()
over it. You leak on error.

> +        if (!(image->palette = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 256 * sizeof(*image->palette))))
> +        {
> +            WARN("Not enough memory to allocate image palette, returning NULL.\n");
> +            return NULL;
> +        }
Leak on error.

>  static HRESULT WINAPI d3drm_texture1_InitFromFile(IDirect3DRMTexture *iface, const char *filename)
>  {
> -    FIXME("iface %p, filename %s stub!\n", iface, debugstr_a(filename));
> +    struct d3drm_texture *object = impl_from_IDirect3DRMTexture(iface);
I'd suggest "texture" instead of "object".

> +    if (object->d3drm)
> +    {
> +        /* InitFromFile seems to AddRef IDirect3DRM even if it fails. */
> +        IDirect3DRM_AddRef(object->d3drm);
> +    }
This is very suspicious, since you always assign "object->d3drm" after
calling InitFromFile(). It's not backed up by tests either.