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

[Aaryaman Vasishta]

On Fri, May 13, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
> On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta(a)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