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 <martin(a)martin.st> 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 <martin(a)martin.st> 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ö <martin(a)martin.st> 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ö <martin(a)martin.st> 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