[PATCH 5/6] d3drm: Partially implement IDirect3DRMTexture*::InitFromFile. (v2 resend)

[Henri Verbeet]

On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta at gmail.com> wrote:
> @@ -47,7 +48,7 @@ static inline struct d3drm_texture *impl_from_IDirect3DRMTexture3(IDirect3DRMTex
>  void d3drm_texture_destroy(struct d3drm_texture *texture)
>  {
>      d3drm_object_cleanup((IDirect3DRMObject*)&texture->IDirect3DRMTexture3_iface, &texture->obj);
> -    if (texture->image)
> +    if (texture->image && texture->d3drm)
>          IDirect3DRM_Release(texture->d3drm);
I guess this and the similar change in d3drm_texture3_InitFromImage()
are because LoadTexture() still uses d3drm_texture_create(). Is there
any reason you can't make the changes to LoadTexture() before this
patch? (Or even as the first patch in the series, for that matter?)

> +static void CDECL destroy_image_callback(IDirect3DRMObject *obj, void *arg)
> +{
> +    struct d3drm_texture *texture = arg;
> +    HeapFree(GetProcessHeap(), 0, texture->image->buffer1);
> +    HeapFree(GetProcessHeap(), 0, texture->image);
> +}
I suppose it doesn't matter much, but passing the image would be sufficient.

> +    if (bpl * h > UINT_MAX)
> +    {
> +        hr = D3DRMERR_BADALLOC;
> +        goto cleanup;
> +    }
Did you test this does what you think it does?

> +        if (image)
> +        {
> +            if (image->buffer1)
> +                HeapFree(GetProcessHeap(), 0, image->buffer1);
> +            HeapFree(GetProcessHeap(), 0, image);
> +        }
HeapFree() handles NULL pointers fine. I.e.:
    if (image)
        HeapFree(GetProcessHeap(), 0, image->buffer1);
    HeapFree(GetProcessHeap(), 0, image);

>  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 *texture = impl_from_IDirect3DRMTexture(iface);
> +    D3DRMIMAGE *image;
> +    HRESULT hr;
>
> -    return E_NOTIMPL;
> +    TRACE("iface %p, filename %s.\n", iface, debugstr_a(filename));
> +
> +    if (texture->image)
> +    {
> +        /* d3drm intentionally leaks a reference IDirect3DRM if texture is already initialized. */
> +        IDirect3DRM_AddRef(texture->d3drm);
> +        return D3DRMERR_BADOBJECT;
> +    }
> +    if (FAILED(hr = d3drm_texture_load(texture, filename, FALSE, &image)))
> +        return hr;
> +
> +    hr = IDirect3DRMTexture3_InitFromImage(&texture->IDirect3DRMTexture3_iface, image);
> +
> +    return hr;
>  }
This looks a bit awkward. Why do you need the "if (texture->image)"
block? InitFromImage() should leak the d3drm reference fine on its
own.