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

[Aaryaman Vasishta]

On Fri, May 13, 2016 at 3:55 PM, Henri Verbeet  wrote:
> 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?)

No real reason. I do agree that they feel like hacks (just to make sure the
tests don't fail). I'll add a separate patch in the beginning to rectify
this.
> > +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.

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

Right, bpl * h can overflow UINT_MAX. It should be if (bpl > UINT_MAX / h).
The error returned is correct, as discussed in the v2 of this patch (see
https://www.winehq.org/pipermail/wine-devel/2016-March/112457.html, your
reply comes later on in the thread)
> > -
> >    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);

Will do. I became a bit over-cautious about handling NULL here.
> > - 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

I did it so that it wouldn't call d3drm_texture_load only to realize that
the image is already initialized when InitFromImage is called. I could
probably add that check within d3drm_texture_load, though.

Cheers,
Aaryaman