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

On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta@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