Re: [resend v2 4/4] ddraw: Do refcounting only through wined3d_texture_{inc, dec}ref().
On 22 December 2015 at 22:43, Riccardo Bortolato <rikyz619(a)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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWiWv3AAoJEN0/YqbEcdMwOMQP/i+pgpQqJZN8RjL+v3Per0Zp cIQ2yDPuuqSi+eVnNuRcHGyIG8CMbAcH1qZSupTPnMSuySrxQlClpKJbkwmhHLdU ePnXT2JVOw78Mt6DAohsQ8D4E4FjwG4Zun6bOe7ErSvmzWiizxyw1gHg3Jw/ptbp /EROaT0LONV1Hl6iSF9taWCh2+DMPCXnEx9KIGJnxo75HWGn564BYJfdJH3hZgMC 25utaHwTsgHhIIael0CWDEh80wkKVU/xoBP6QKuGhzVlZpO6p2NgiE6+zueOjqyu spf8QwdQlPAZ3uB6m2Vl1lDuhH23asTHSULunY83nIpQqLmUY2r3stifHZurpWFE 9acwfrM1AVAKIi0zR+gfU8xqphkzF2awF365h50FcyLxK2q0AHfiiTcnDRoktYlI hixV20cuY8vVY6Jt0RSqNdQrk5RyOUo0FtslKX3pc/sGLZTGeUM/WVTcTVqGDjYk yE8b5YHmQzuIBx7kgfkzdsgvBWQJ3LJMPfhxKMk5eoBFP0HpuqKJuSsTF6nEcaF1 7bYm9YNhHkUMvsaG3HllpAhzG5huI/rahco8qTQPISFV+NG3/hfpgQvMn6msHl0x XGGUHfKYYG+Sr9dd00Ik3em6do1LzV5er8EjNGE5kL5bhgphmYr20pgpcfStNlWt bb0MFiVf3PGIhNlPchH+ =ul3N -----END PGP SIGNATURE-----
On 3 January 2016 at 19:44, Stefan Dösinger <stefandoesinger(a)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) -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWiXYQAAoJEN0/YqbEcdMwlbwP/2w+95KBI9a01PZrzpebc4qq U6ZReNr6d/59GY676nU92tcttzbdL5sZOd/AiFZIwMCZyqAUOA5VdZnJxmXykIp4 kCxq72JV5LsX7jyFzgaZxt8n0aZdQQl+AUjvDCMnkU61gBGJYk/+BpAE+zAxuGMY vygB4B7PU9Owzcm1WlhJPwpBErmcukBFyyOly2v5BDg35k8ZoXzGCyOtWjo4EE8m TSnFBfMeptupMp/uKeK5iV9F1oq0z00nFMpSOBmmHjO10cq41c++9J5sd69WbCTY p0G1jDTY4BGgL6ZQ1WtZTggXSaDYDlA0U/g+pYAH92V5PyPbYloqdR88XLGlAGaI LKtJxYo3Rq59jc32Oba+qS3cTJg1R4AYgB6E3hh8RTKgNq3xk+yvIHrSDDCTMYsw pBLqcvDdd9UOMOB4lGbwKnFUZDhcP1Yix7i1D4npveELdZKbrDqPnKj6I0YbeDC4 33uGThU2ERuJz+DdYDCLkOBwexRv8Ybcohyfsb9V0GNbfen2+1MPhRzwhKcDVYW4 v4O8/w6ikMxgeMuHvY+RB8rkZXo+wfU+7gKFzV81IoQG4Eia7bNVkehwaC9Bj+Lr AxpWDvGMhM3DHXZ0EDg2YQPiHpKS36MLg2848ti8gyCljZhAdbCc2Mt9DYxZn59S 9nrSPfGQ+v05Tobud/AH =t7l6 -----END PGP SIGNATURE-----
On 3 January 2016 at 20:27, Stefan Dösinger <stefandoesinger(a)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(a)gmail.com>:
On 22 December 2015 at 22:43, Riccardo Bortolato <rikyz619(a)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
participants (3)
-
Henri Verbeet -
Riccardo Bortolato -
Stefan Dösinger