On Sun, Jul 26, 2015 at 4:33 PM, Stefan Dösinger <stefandoesinger@gmail.com> wrote:

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

Hi,

I like the overall structure a lot more than the previous incarnation!

Here are a few more comments:

> + hr = DirectDrawCreate(NULL, &ddraw, NULL);
> + if (FAILED(hr))
> + return hr;
> +
> hr = Direct3DRMDevice_create(&object);
> if (FAILED(hr))
> return hr;
You're leaking "ddraw" here. There are many more incomplete error handling paths in the patches.

I'll post another incarnation with all the leaks I can possibly find fixed :)

> +HRESULT set_clipper_surface(struct d3drm_device *object, IDirectDraw *ddraw, IDirectDrawClipper *clipper, int width, int height,
> + IDirectDrawSurface **surface) DECLSPEC_HIDDEN;
I'm not entirely happy with this name, because it doesn't merely set any surfaces, it creates them. What do you think about d3drm_device_create_surfaces_from_clipper? Check for consistency wrt the "d3drm_device" part. E.g. the next function you could name "d3drm_device_init" or just "device_init".

It took me a while to figure out a name, since a name like d3drm_device_create_surfaces_from_clipper felt too long for me initially. But if long function names are accepted then I don't mind putting that name up.

> +HRESULT init_device(struct d3drm_device *device, UINT version, IDirect3DRM *d3drm, IDirectDraw *ddraw, IDirectDrawSurface *surface, BOOL z_surface, IDirect3D2 *d3d2,
> + int width, int height)
"BOOL z_surface" is unused right now. It won't be needed until you add CreateDeviceFromSurface version 3.

Right, I'll remove this for now and re-add it in the CreateDeviceFromSurface patches. Note that z_surface is still needed in version 1 and 2 of CreateDeviceFromSurface in the cases where the surface already has ds attached to it by the application (in which case native wouldn't create its own ds internally, as seen by the tests).

You are leaking ds here in the success case. You can release it after calling AddAttachedSurface.

I'm not sure why I allowed that leak to happen, but I believe it was causing some refcounting/segfault issues if I released the ds there. I'll get back with more details about this.

Jam