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

[Henri Verbeet]

On 11 May 2016 at 21:19, Aaryaman Vasishta  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?)
> +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.
> - if (bpl * h > UINT_MAX)
> - {
> -
>    hr = D3DRMERR_BADALLOC;
> -
>    goto cleanup;
> - }

Did you test this does what you think it does?
> -
>    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);
> static HRESULT WINAPI d3drm_texture1_InitFromFile(IDirect3DRMTexture *iface, const char *filename)
>  {
> - FIXME("iface %p, filename %s stub!\n", iface, debugstr_a(filename));
> - struct d3drm_texture *texture = impl_from_IDirect3DRMTexture(iface);
> - D3DRMIMAGE *image;
> - HRESULT hr;
> - return E_NOTIMPL;
> - TRACE("iface %p, filename %s.\n", iface, debugstr_a(filename));
> -
> - 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.

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

[Aaryaman Vasishta]

On Fri, May 13, 2016 at 3:55 PM, Henri Verbeet  wrote:
> On 11 May 2016 at 21:19, Aaryaman Vasishta 
> 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

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

[Henri Verbeet]

On 13 May 2016 at 17:52, Aaryaman Vasishta  wrote:
> 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.

It's usually not worth optimising error paths, unless you happen to
have an application that needs it.

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

[Aaryaman Vasishta]

Right, will keep that in mind and resend this patch. But I'm curious as to
how does an application depend on optimised error paths? If I understood
correctly, this optimisation would probably be needed for games who have a
lot of failed texture loading calls per frame (even on windows), and not
optimising this would cause it to lag a lot. Or is it something else?

Cheers,
Aaryaman

On Fri, May 13, 2016 at 9:28 PM, Henri Verbeet  wrote:
> On 13 May 2016 at 17:52, Aaryaman Vasishta 
> wrote:
> > 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.
>
> It's usually not worth optimising error paths, unless you happen to
> have an application that needs it.

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

[Henri Verbeet]

On 13 May 2016 at 18:11, Aaryaman Vasishta  wrote:
> Right, will keep that in mind and resend this patch. But I'm curious as to
> how does an application depend on optimised error paths? If I understood
> correctly, this optimisation would probably be needed for games who have a
> lot of failed texture loading calls per frame (even on windows), and not
> optimising this would cause it to lag a lot. Or is it something else?

Something along those lines, yeah. You could e.g. have an application
that does redundant InitFromFile() calls inside of some hot-path
because instead of keeping track of whether the texture is initialised
it depends on InitFromFile() failing quickly. Note that that only
works if it fails quickly on Windows as well, and leaking the
IDirect3DRM reference on each failed call makes the scenario a bit
unlikely.