On Thu, Jul 9, 2015 at 5:20 PM, Stefan Dösinger <stefandoesinger@gmail.com> wrote:

Is there really a need to store (and AddRef) the clipper? The primary surface already keeps a reference to it, so you should be OK. It won't give you the increment by 2 that native has though, but I don't think that's important.

At the moment there doesn't seem to be a significant need, except for the refcounting requirements.

> + hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &ds, NULL);
> + if (FAILED(hr))
> + return hr;
> +
> + hr = IDirectDrawSurface_AddAttachedSurface(surface, ds);
> + if (FAILED(hr))
> + {
> + IDirectDrawSurface_Release(ds);
> + return hr;
> + }
Similarly: Is there a need to store the depth stencil? You could just release your reference after attaching it to the render target, it will stay alive anyway. (If it's just for the DeleteAttachedSurface later: Ignore it)

It's just for the DeleteAttachedSurface part. I'll remove it for now so it's easier to thunk towards version 3.

You have to keep ddraw alive because nothing (neither your surfaces nor your device) hold a reference to it. Same for the primary. I think it's also good to store a reference to the render target, I believe you need it to show the rendering results on the screen (The immediate mode device holds a ref too, but it's awkward to always query it from the device).

> + if (device->ref == 1 && device->z_created_internally)
> + IDirectDrawSurface_DeleteAttachedSurface(device->render_target, 0, device->depth_surface);
As mentioned on IRC this is not thread safe A lot can happen between your ref == 1 comparison and the actual InterlockedDecrement.

Since we're not gonna use DeleteAttachedSurface, we won't need to read the refcount either. I agree there are some synchronization issues in this case. A good idea for future reference would be to use flags to determine which version the device was created on, to make it thread-safe and generic enough for the release method to be used by all versions.

Wrt patch 6 (d3drm1_CreateDeviceFromClipper): It's a bit unfortunate that this is mostly a copypasted version of version 3, and the difference that actually matters is in init_device anyway. Maybe there is a nicer solution, e.g. a helper function for v1 and v3 with a version parameter.

Similar to set_primary_surface defined above init_device in the patches (though we won't need the version parameter there anymore as I'll be using the struct there.).

I've taken note of all the other suggestions and will incorporate them in the next patchset. I do agree there's quite a bit of optimization left with regards to the thunking and version checks.

Thanks!

Jam