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

[Aaryaman Vasishta]

On Fri, May 13, 2016 at 3:55 PM, Henri Verbeet <hverbeet at gmail.com> wrote:

> 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?)
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160513/fedf9012/attachment.html>