Re: [PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Henri Verbeet]

On 13 February 2014 13:05, Martin Storsjo  wrote:
> static HRESULT arbfp_blit_alloc(struct wined3d_device *device)
>  {
> - struct arbfp_blit_priv *priv;
> device->blit_priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct arbfp_blit_priv));
> if(!device->blit_priv) {
>     ERR("Out of memory\n");
>     return E_OUTOFMEMORY;
> }
> - priv = device->blit_priv;
> -
> - if (wine_rb_init(&priv->shaders, &wined3d_arbfp_blit_rb_functions) == -1)
> - {
> -
>    ERR("Failed to initialize rbtree.\n");
> -
>    HeapFree(GetProcessHeap(), 0, device->blit_priv);
> -
>    device->blit_priv = NULL;
> -
>    return E_OUTOFMEMORY;
> - }
> - return WINED3D_OK;
>
> }

The following is probably slightly nicer:

static HRESULT arbfp_blit_alloc(struct wined3d_device *device)
 {
-    device->blit_priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
sizeof(struct arbfp_blit_priv));
-    if(!device->blit_priv) {
-        ERR("Out of memory\n");
+    struct arbfp_blit_priv *priv;
+
+    if (!(priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*priv))))
         return E_OUTOFMEMORY;
+
+    if (wine_rb_init(&priv->shaders, &wined3d_arbfp_blit_rb_functions) == -1)
+    {
+        ERR("Failed to initialize rbtree.\n");
+        HeapFree(GetProcessHeap(), 0, priv);
+        return E_FAIL;
     }
+
+    device->blit_priv = priv;
+
     return WINED3D_OK;
 }
> - if (desc)
> -
>    HeapFree(GetProcessHeap(), 0, desc);

There's no point in checking for NULL before HeapFree(). Perhaps more
importantly, I think it makes more sense to insert the rbtree entries
in arbfp_blit_set(), instead of duplicating the code between
gen_p8_shader() and gen_yuv_shader().

[PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Martin Storsjö]

On Fri, 14 Feb 2014, Henri Verbeet wrote:
> On 13 February 2014 13:05, Martin Storsjo  wrote:
> > static HRESULT arbfp_blit_alloc(struct wined3d_device *device)
> >  {
> > - struct arbfp_blit_priv *priv;
> > device->blit_priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct arbfp_blit_priv));
> > if(!device->blit_priv) {
> >     ERR("Out of memory\n");
> >     return E_OUTOFMEMORY;
> > }
> > - priv = device->blit_priv;
> > -
> > - if (wine_rb_init(&priv->shaders, &wined3d_arbfp_blit_rb_functions) == -1)
> > - {
> > -
> >    ERR("Failed to initialize rbtree.\n");
> > -
> >    HeapFree(GetProcessHeap(), 0, device->blit_priv);
> > -
> >    device->blit_priv = NULL;
> > -
> >    return E_OUTOFMEMORY;
> > - }
> > - return WINED3D_OK;
> >
> > }
>
> The following is probably slightly nicer:
>
> static HRESULT arbfp_blit_alloc(struct wined3d_device *device)
> {
> - device->blit_priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
>
> sizeof(struct arbfp_blit_priv));
> - if(!device->blit_priv) {
> -
>    ERR("Out of memory\n");
> - struct arbfp_blit_priv *priv;
> -
> - if (!(priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*priv))))
>    return E_OUTOFMEMORY;
> -
> - if (wine_rb_init(&priv->shaders, &wined3d_arbfp_blit_rb_functions) == -1)
> - {
> -
>    ERR("Failed to initialize rbtree.\n");
> -
>    HeapFree(GetProcessHeap(), 0, priv);
> -
>    return E_FAIL;
> }
> -
> - device->blit_priv = priv;
> - return WINED3D_OK;
>
> }

That's nicer indeed, updated the patch accordingly locally.
> > - if (desc)
> > -
> >    HeapFree(GetProcessHeap(), 0, desc);
>
> There's no point in checking for NULL before HeapFree().

MSDN says "If this pointer is NULL, the behavior is undefined" about the 
lpMem parameter to HeapFree.
> Perhaps more importantly, I think it makes more sense to insert the 
> rbtree entries in arbfp_blit_set(), instead of duplicating the code 
> between gen_p8_shader() and gen_yuv_shader().

Good point, I've updated my patch accordingly.

I'll resend all three patches in a little while unless you or Stefan have 
got more comments at the moment.

// Martin

[PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Bruno Jesus]

On Mon, Feb 17, 2014 at 9:21 AM, Martin Storsjö  wrote:
> > > - if (desc)
> > > -
> > >    HeapFree(GetProcessHeap(), 0, desc);
> >
> > There's no point in checking for NULL before HeapFree().
>
> MSDN says "If this pointer is NULL, the behavior is undefined" about the
> lpMem parameter to HeapFree.

Actually it does not matter what MSDN says in this case, the wine
source code is very clear:
http://source.winehq.org/source/dlls/ntdll/heap.c#L1740
> // Martin

Bruno

[PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Martin Storsjö]

On Mon, 17 Feb 2014, Bruno Jesus wrote:
> On Mon, Feb 17, 2014 at 9:21 AM, Martin Storsjö  wrote:
> > > > - if (desc)
> > > > -
> > > >    HeapFree(GetProcessHeap(), 0, desc);
> > >
> > > There's no point in checking for NULL before HeapFree().
> >
> > MSDN says "If this pointer is NULL, the behavior is undefined" about the
> > lpMem parameter to HeapFree.
>
> Actually it does not matter what MSDN says in this case, the wine
> source code is very clear:
> http://source.winehq.org/source/dlls/ntdll/heap.c#L1740

Yes, but isn't the wined3d code supposed to be able to run on top of plain 
windows as well, not only on top of wine?

// Martin

[PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-02-17 13:29, schrieb Martin Storsjö:
> Yes, but isn't the wined3d code supposed to be able to run on top
> of plain windows as well, not only on top of wine?

Yes, but Windows behaves the same way. We should have tests for that
somewhere in kernel32/tests, and all our code (tests and otherwise)
assumes that HeapFree(, , NULL) is a no-op.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTAgIhAAoJEN0/YqbEcdMwTHkQAIBEGqDkFrkDxBKAFGHd5ZIV
vQ4BBv8FzCUDc3Jv7VkqIqgboVfcIcxTR/WXqJ9aAyoUOVAot3fgmGW2aCNINFL/
JHaNqOIaAJdBgvNc8VSa2Pj2L+82Co8RpbBMFQBY1vCBVjfH6MPvxwN5yXv9D9UM
kLGuVZwOG2y8HAM1DikISjGqG0CLTrbRsc1DC4OV+EvrKBCHHRPGWZu67+idkNTq
nyBhzkkEL3ijKn04tvvnuNEnfofAO7/REXgCiNj2mjkaNDmARUwnjVRnD9NUV5Q/
HrTaLIAM854HS3zCScrINuiG4NTdXT/ag4JCzxQmKEttwiK9Fly43k2J6+H31vKb
NWREMmvKBrVFeQE2rGKuzJ2yQtUWWwUGqLxtIyIK9w5hF0TMJY8dtyzfadq6SKZY
Tm5Ezd1hGFL1O+X61k0vqEZpX9EGdkLZetuoAkEI32HfNW5aCcbmPV7I7LFBJi8S
ZY1JizytJPJt6PZKGmKSHM3e4hnpfNKTb81Xd4mP/pCTWE2pUwiPyuA0i2jMuTMp
yzZLSnnLEd2ezjwh6fSFzuQlHnirUEnjhNo2UqZbcmZ+83Aug/vuv6cCkM5pT7J0
b+yiRRo3SxYFVs7ia7Cvem0OqbB3PRae3ZNIsk0w/BKJdkDhu6C8LPg/0vU0G4lL
HJtZE9e0VRVKaS5y/E7p
=QBS+
-----END PGP SIGNATURE-----

[PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Martin Storsjö]

On Mon, 17 Feb 2014, Stefan Dösinger wrote:
> Am 2014-02-17 13:29, schrieb Martin Storsjö:
> > Yes, but isn't the wined3d code supposed to be able to run on top
> > of plain windows as well, not only on top of wine?
>
> Yes, but Windows behaves the same way. We should have tests for that
> somewhere in kernel32/tests, and all our code (tests and otherwise)
> assumes that HeapFree(, , NULL) is a no-op.

Ok then, that makes me more comfortable with doing it like that.

I've resent the set with the remaining patch taking all the comments into 
consideration now.

// Martin