[PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Aaryaman Vasishta]

On Tue, Mar 29, 2016 at 4:59 PM, Henri Verbeet <hverbeet at gmail.com> wrote:

> On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta at gmail.com>
> wrote:
> > v3: Add bitmap verification, use InitFromFile and other minor
> improvements.
> >
> Implementing InitFromFile() should be a separate patch from
> implementing LoadTexture().
>
> >      if (FAILED(hr = d3drm_texture_create(&object)))
> >          return hr;
> >
> > +    if (FAILED(hr =
> IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface,
> filename)))
> > +    {
> > +        d3drm_texture_destroy(object);
> > +        return hr;
> > +    }
> > +
> > +    object->d3drm = iface;
> > +    IDirect3DRM_AddRef(iface);
> It would be good to back this up with tests. It would seem more
> natural to pass the IDirect3DRM interface to d3drm_texture_create().
> If that's how it's supposed to work, you can make that change in a
> separate patch.
>
Right.

>
> > @@ -39,6 +40,9 @@ struct d3drm_texture
> >      IDirect3DRMTexture IDirect3DRMTexture_iface;
> >      IDirect3DRMTexture2 IDirect3DRMTexture2_iface;
> >      IDirect3DRMTexture3 IDirect3DRMTexture3_iface;
> > +    IDirect3DRM *d3drm;
> > +    D3DRMIMAGE *image;
> > +    BOOL internal_image_created;
> You don't really need internal_image_created, you can probably just
> add a destroy callback.
>
Are there any existing examples within wine where a destroy callback was
used in similar cases?

> This is very suspicious, since you always assign "object->d3drm" after
> calling InitFromFile(). It's not backed up by tests either.
>
I think I had put that while debugging a segfault while the tests ran on
wine. LoadTexture called InitFromFile first, which then AddRef'd
IDirect3DRM. Since IDirect3DRM member wasn't initialized in the object by
then, the if check is needed. Also since LoadTexture needs to AddRef only
after IDirect3DRM after it's successfully loaded, I put the AddRef below
d3drm_texture_load.

I agree with all the other changes you mentioned. The leaks and signed
local vars are definitely a problem. I'll fix them, divide the patches up
and begin work on CreateObject first before submitting these again.


Cheers,
Aaryaman
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160329/af3c0da9/attachment.html>