Re: DDRAW: Fix reference counting

[Robert Shearman]

Stefan Dösinger wrote:
> Hello,
> This patch increases the reference count of a DirectDraw object when a Surface 
> is created, like Windows does. It also adds a test which tests this 
> functionality.
>
> Stefan Dösinger
> Index: dlls/ddraw/ddraw_main.c
>
> RCS file: /home/wine/wine/dlls/ddraw/ddraw_main.c,v
> retrieving revision 1.9
> diff -u -r1.9 ddraw_main.c
> --- dlls/ddraw/ddraw_main.c	12 Sep 2005 14:12:47 -0000	1.9
> +++ dlls/ddraw/ddraw_main.c	18 Sep 2005 19:56:12 -0000
> @@ -697,8 +697,10 @@
>     }
>     else if (pDDSD->ddsCaps.dwCaps & DDSCAPS_TEXTURE)
>     {
> - /* create texture */
> - /* create texture. This increases the DD refcount by 2 */

Why do you increase the refcount by 2? Since you add and release them in 
the same functions, it is not really necessary and just adds complexity 
to the code.
> hr = create_texture(This, pDDSD, ppSurf, pUnkOuter);
> - if(!FAILED(hr))
> -
> IDirectDraw7_AddRef(iface); /* Plus the increase at the end of the function */

if (!FAILED(hr)) -> if (SUCCEEDED(hr))

DDRAW: Fix reference counting

[Stefan Dösinger]

Hi,
> Why do you increase the refcount by 2? Since you add and release them in
> the same functions, it is not really necessary and just adds complexity
> to the code.

Well, Windows does so for some reason, and after what I've seen with Empire 
Earth, I suspect that there are some apps out there which depend on things 
like this. It's unlikely, but I would not be surprised if that happens.
> if (!FAILED(hr)) -> if (SUCCEEDED(hr))

Could have guesses this one. Thanks.

Thanks, for your suggestions,
Stefan

DDRAW: Fix reference counting

On Mon, Sep 19, 2005 at 09:27:05AM +0200, Stefan D?singer wrote:
> Hi,
> > Why do you increase the refcount by 2? Since you add and release them in
> > the same functions, it is not really necessary and just adds complexity
> > to the code.
>
> Well, Windows does so for some reason, and after what I've seen with Empire 
> Earth, I suspect that there are some apps out there which depend on things 
> like this. It's unlikely, but I would not be surprised if that happens.

In that case, it would be really beneficial with a unit test.
Both as documentation and to see what Windows does.

DDRAW: Fix reference counting

[Stefan Dösinger]

Hello,
> In that case, it would be really beneficial with a unit test.
> Both as documentation and to see what Windows does.

Excuse my ignorance, but what exactly is meant with 'unit test'? As far as 
I've learned, it's a small piece of code which uses this functionality and 
checks the results. My patch includes such a test:

--- /dev/null	2005-09-01 15:22:32.000000000 +0200
+++ dlls/ddraw/tests/refcount.c	2005-09-18 21:57:52.000000000 +0200
<...>
+    /* Create a Texture. Increases the refcount by 2!!!*/
+    memset(&ddsd, 0, sizeof(ddsd));
+    ddsd.dwSize = sizeof(ddsd);
+    ddsd.dwFlags = DDSD_CAPS | DDSD_HEIGHT | DDSD_WIDTH | DDSD_PIXELFORMAT;
+    ddsd.ddsCaps.dwCaps = DDSCAPS_TEXTURE;
<...>
+    rc = IDirectDraw7_CreateSurface( lpDD, &ddsd, &lpDDS, NULL );
+    ok(rc == DD_OK, "CreateSurface for a mipmap surface returned %lx\n", rc);
+    if(rc == DD_OK)
+    {
+        newref = IDirectDraw_AddRef(lpDD) - 1;
+        IDirectDraw_Release(lpDD);
+        ok( (newref - oldref) == 2, "DD reference count increased by %d 
during creation of a mipmap surface\n", newref - oldref);
+        oldref = newref;
+
+        newref = IDirectDrawSurface_AddRef(lpDDS) - 1;
+        IDirectDrawSurface_Release(lpDDS);
+        ok(newref == 1, "Initial surface reference count of a mipmap surface 
is %d\n", newref);
+    
+        /* Release the surface */
+        IDirectDrawSurface_Release(lpDDS);
+
+        newref = IDirectDraw_AddRef(lpDD) - 1;
+        IDirectDraw_Release(lpDD);
+        ok( (newref - oldref) == -2, "DD reference count decreased by %d 
during releasing a mipmap surface\n", newref - oldref);
+    }

Is that enought? This test succeeds in Windows 2000 and Windows 95.

Stefan

DDRAW: Fix reference counting

[Jakob Eriksson]

Stefan Dösinger wrote:
> Hello,
> > In that case, it would be really beneficial with a unit test.
> > Both as documentation and to see what Windows does.
>
> Excuse my ignorance, but what exactly is meant with 'unit test'? As far as 
> I've learned, it's a small piece of code which uses this functionality and 
> checks the results. My patch includes such a test:

I'm sorry. The test looks excellent.

//Jakob