May 2016 - Wine-Devel - list.winehq.org

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

by Henri Verbeet

On 13 May 2016 at 18:11, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> 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.

May 13, 2016

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

by 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 <hverbeet(a)gmail.com> wrote: > On 13 May 2016 at 17:52, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > 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. >

May 13, 2016

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

by Henri Verbeet

On 13 May 2016 at 17:52, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> 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.

May 13, 2016

Re: [PATCH 3/6] d3drm: Implement IDirect3DRM*_CreateTexture. (v5)

by Aaryaman Vasishta

Right, will fix this, and resend the patch series with the other fixes as well. Cheers, Aaryaman On Fri, May 13, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > + if (FAILED(hr = IDirect3DRMTexture3_InitFromImage(*texture, image))) > > + { > > + IDirect3DRMTexture3_Release(*texture); > > + return D3DRMERR_BADVALUE; > > + } > You're not using the value of "hr", so the assignment is redundant. > > >

May 13, 2016

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

by Aaryaman Vasishta

On Fri, May 13, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta(a)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

May 13, 2016

Re: [PATCH v2 4/6] ddraw/tests: Test creating flippable cubemaps.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=22978 Your paranoid android. === w1064 (32 bit ddraw7) === ddraw7.c:2275: Test failed: Expected {0, 0, 512, 384}, got {0, 0, 1024, 768}. === w1064 (64 bit ddraw7) === ddraw7.c:2275: Test failed: Expected {0, 0, 512, 384}, got {0, 0, 1024, 768}.

May 13, 2016

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

by Henri Verbeet

On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta(a)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?) > +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.

May 13, 2016

Re: [PATCH 3/6] d3drm: Implement IDirect3DRM*_CreateTexture. (v5)

by Henri Verbeet

On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > + if (FAILED(hr = IDirect3DRMTexture3_InitFromImage(*texture, image))) > + { > + IDirect3DRMTexture3_Release(*texture); > + return D3DRMERR_BADVALUE; > + } You're not using the value of "hr", so the assignment is redundant.

May 13, 2016

Re: [PATCH 1/6] d3drm: Introduce d3drm_texture_destroy helper (v3).

by Henri Verbeet

On 11 May 2016 at 21:19, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > +void d3drm_texture_destroy(struct d3drm_texture *texture) This should be static. I.e.: +static void d3drm_texture_destroy(struct d3drm_texture *texture) Adding static prevents the function from being available outside of texture.c. (See also http://port70.net/~nsz/c/c89/c89-draft.html#3.1.2.2 for the details, although "The C Programming Language" by Kernighan & Ritchie makes for better reading.) Applies to d3drm_validate_image() as well.

May 13, 2016

Re: mciavi32: Propely handle Heap[Re]Alloc failure (resend)

by Kirill Smirnov

On 12.05.2016 22:39, Andrew Eikum wrote: > Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> > > Thanks. Just wondering, does this fix an issue in an application, or > you just happened to notice it? > > Andrew Just playing with clang static analyzer, nothing more :-) Unlike other patches in the series of clang-spotted, this one looked a little bit deeper than just "dereference of NULL pointer". That's why I did not stamped it with "spotted by clang" mark. -- Kirill

May 12, 2016