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