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

On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta@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