On 22 December 2015 at 22:43, Riccardo Bortolato rikyz619@gmail.com wrote:
@@ -1227,6 +1221,7 @@ static HRESULT WINAPI DECLSPEC_HOTPATCH ddraw_surface7_Flip(IDirectDrawSurface7 struct wined3d_texture *texture; IDirectDrawSurface7 *current; struct wined3d_surface *tmp;
- unsigned int tmp_idx;
You shouldn't need that. Flippable surfaces aren't supposed to be textures, and only textures should have mip-levels. It should be enough to validate the sub-resource index is 0 and print an ERR if it somehow isn't. I don't think we enforce that DDSCAPS_MIPMAP needs DDSCAPS_TEXTURE. That would be a bug and needs tests.
@@ -6286,8 +6287,10 @@ void ddraw_surface_init(struct ddraw_surface *surface, struct ddraw *ddraw, stru } desc->lpSurface = NULL;
- wined3d_surface_incref(wined3d_surface);
- wined3d_texture_incref(wined3d_texture); surface->wined3d_surface = wined3d_surface;
- surface->wined3d_texture = wined3d_texture;
The reference counting here is odd. You'd normally increment the reference count right before or after storing the pointer.
You also store wined3d_surface without increasing the reference count, although that's probably ok because it's protected by the one from the texture. Still, it would probably be nicer to just get rid of the wined3d_surface field first.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2016-01-03 um 18:06 schrieb Henri Verbeet:
- wined3d_surface_incref(wined3d_surface);
- wined3d_texture_incref(wined3d_texture); surface->wined3d_surface = wined3d_surface;
- surface->wined3d_texture = wined3d_texture;
The reference counting here is odd. You'd normally increment the reference count right before or after storing the pointer.
This goes back to one of my suggestions: The individual subresources already hold a reference to the texture. This decref removes the initial ref from the texture_create call. Doing it here avoids special treatment of subresource 0 in other places.
Though I guess a comment explaining this might be a reasonable thing to do.
On 3 January 2016 at 19:44, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 2016-01-03 um 18:06 schrieb Henri Verbeet:
- wined3d_surface_incref(wined3d_surface);
- wined3d_texture_incref(wined3d_texture); surface->wined3d_surface = wined3d_surface;
- surface->wined3d_texture = wined3d_texture;
The reference counting here is odd. You'd normally increment the reference count right before or after storing the pointer.
This goes back to one of my suggestions: The individual subresources already hold a reference to the texture. This decref removes the initial ref from the texture_create call.
Are you perhaps thinking about the wined3d_texture_decref() call in ddraw_surface_create()? That one makes sense, but it's not what I commented on.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2016-01-03 um 18:50 schrieb Henri Verbeet:
Are you perhaps thinking about the wined3d_texture_decref() call in ddraw_surface_create()? That one makes sense, but it's not what I commented on.
Er yes, I didn't read your comment carefully enough. (When I reviewed the patch the decref call seemed odd at first sight, which is why I was thinking you're talking about it too)
Is the issue you mention here the ordering of the 3 lines, or am I missing something else? (I'll let Riccardo comment on the "get rid of wined3d_surface first part, afair he had some more patches for that that depend on this patch)
On 3 January 2016 at 20:27, Stefan Dösinger stefandoesinger@gmail.com wrote:
Is the issue you mention here the ordering of the 3 lines, or am I missing something else?
Yeah, just the ordering being weird.
2016-01-03 19:06 GMT+01:00 Henri Verbeet hverbeet@gmail.com:
On 22 December 2015 at 22:43, Riccardo Bortolato rikyz619@gmail.com wrote:
@@ -1227,6 +1221,7 @@ static HRESULT WINAPI DECLSPEC_HOTPATCH ddraw_surface7_Flip(IDirectDrawSurface7 struct wined3d_texture *texture; IDirectDrawSurface7 *current; struct wined3d_surface *tmp;
- unsigned int tmp_idx;
You shouldn't need that. Flippable surfaces aren't supposed to be textures, and only textures should have mip-levels. It should be enough to validate the sub-resource index is 0 and print an ERR if it somehow isn't. I don't think we enforce that DDSCAPS_MIPMAP needs DDSCAPS_TEXTURE. That would be a bug and needs tests.
Hi Henri,
I know it should be 0 but the idea here is to keep the current functionality and eventual issues or quirks that may or may not be contained within the already existing code.
@@ -6286,8 +6287,10 @@ void ddraw_surface_init(struct ddraw_surface *surface, struct ddraw *ddraw, stru } desc->lpSurface = NULL;
- wined3d_surface_incref(wined3d_surface);
- wined3d_texture_incref(wined3d_texture); surface->wined3d_surface = wined3d_surface;
- surface->wined3d_texture = wined3d_texture;
The reference counting here is odd. You'd normally increment the reference count right before or after storing the pointer.
You also store wined3d_surface without increasing the reference count, although that's probably ok because it's protected by the one from the texture. Still, it would probably be nicer to just get rid of the wined3d_surface field first.
I will reorder the lines if that helps, the final goal here is exactly to get rid of wined3d_surface (you can find the whole updated patchset compared against current git master at this address: https://github.com/wine-mirror/wine/compare/master...c10ud:surf6). I can send you the patches but I don't want to clog your inbox or wine-devel.
Ciao, Riccardo
-
Henri Verbeet -
Riccardo Bortolato -
Stefan Dösinger