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

[Henri Verbeet]

On 11 May 2016 at 21:19, Aaryaman Vasishta  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.