Re: [PATCH 5/6] d3drm: Partially implement IDirect3DRMTexture*::InitFromFile. (v2 resend)
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.
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
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.
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.
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.
participants (2)
-
Aaryaman Vasishta -
Henri Verbeet