Re: wined3d: Allow SetCursorProperties on existing cursor
Am Samstag 10 März 2007 06:10 schrieb Erich Hoover:
* Real Name: *>* Erich Hoover *>* Description: *>* The SetCursorProperties call needs to be capable of being performed on *>* existing cursors. The current behavior removes the cursor handle at the *>* beginning of any SetCursorProperties call (and on Uninit3D). The removal *>* of the cursor on SetCursorProperties is counter to the documentation and *>* causes some applications to lose the cursor when SetCursorProperties is *>* re-issued (See Bug #7619 <http://bugs.winehq.org/show_bug.cgi?id=7619>). *>* This patch corrects this issue and permits SetCursorProperties to retrieve *>* the gl texture of an old cursor. *>* Changelog: *>* wined3d: Allow SetCursorProperties on existing cursor *This is not completely correct:
The application can Release the d3d surface after setting the cursor. This would also delete the gl texture used to draw the cursor. Also, the apps can potentially change the surface and call PreLoad. This would change the cursor without SetCursorProperties(The cursor texture does not have to be in D3DPOOL_SCRATCH, so the app can preload it manually).
But you are right, a non-dirty surface causes issues with the current code. I think you can fix that by modifying LoadTexture not to release the surface memory is SFLAG_FORCELOAD is passed. (Or better, fix the SFLAG_DONOTFREE macro in wined3d_private.h). Then, when SetCursorProperties sets the surface texture to 0 remove SFLAG_INTEXTURE from the surface flags.
The API documentation doesn't say anything about an application releasing
the d3d surface, but the documentation in the code seems to indicate that the gl texture needs to remain when the surface is deleted - I did nothing to change this. What I did see as requiring a change is the call to "glDeleteTextures(1, &This->cursorTexture);" at the beginning of SetCursorProperties. Maybe I'm not understanding you correctly, but in my mind it is necessary to remove this call so that applications that re-use the same cursor can do so. This change (in combination with the change for non-dirty surfaces) was also experimentally verified to fix the problem I was experiencing (See Bug #7619<http://bugs.winehq.org/show_bug.cgi?id=7619>). I will look into changing/implementing the flags to eliminate the "non-dirty surface" problem. Erich Hoover ehoover(a)mines.edu
The API documentation doesn't say anything about an application releasing the d3d surface, There is a test for that somewhere in dlls/d3d9/tests/device.c (or some other test file in there).
but the documentation in the code seems to indicate that the gl texture needs to remain when the surface is deleted - I did nothing to change this. But IWineD3DSurfaceImpl_Release will destroy the gl texture of a surface when the surface is destroyed.
Also, if the gl surface still knows the texture, the following could cause an issue: SetCursorProperties(cursorSurface, ...); <render some stuff> LockRect(cursorSurface, ...); <write some thing into the surface> UnlockRect(cursorSurface); PreLoad(cursorSurface); The PreLoad will then change the cursor without SetCursorProperties beeing called. This should not happen. I tested that with some hacky test. Unfortunately this cannot be automated in the visual tests because cursors do not show up in screenshots. This is my hacky cursor test app: http://stud4.tuwien.ac.at/~e0526822/visual.c The successor of that file without the cursor tests is now in dlls/d3d9/tests/visual.c. I think this visual.c version is not the version I used for checking later surface changes.
I ran all of the d3d9 tests and did not get any errors with my patch applied. I did get an error when I ran your modified visual.c test, but I get the same errors with and without my patch. Test before patch:
fixme:d3d:IWineD3DDeviceImpl_GetAvailableTextureMem (0x167ec8) : stub, simulating 64MB for now, returning 64MB left visual.c:191: Test failed: IDirect3DDevice9_ColorFill failed with 88760096 visual.c:212: Test failed: IDirect3DDevice9_SetCursorProperties failed with 8876086c visual: 26 tests executed (0 marked as todo, 2 failures), 0 skipped.
Test after patch:
fixme:d3d:IWineD3DDeviceImpl_GetAvailableTextureMem (0x167ec8) : stub, simulating 64MB for now, returning 64MB left visual.c:191: Test failed: IDirect3DDevice9_ColorFill failed with 88760096 visual.c:212: Test failed: IDirect3DDevice9_SetCursorProperties failed with 8876086c visual: 26 tests executed (0 marked as todo, 2 failures), 0 skipped.
So, which change exactly are you concerned about? Changes are: 1) The removal of "glDeleteTextures(1, &This->cursorTexture);" in device.c This change should just keep SetCursorProperties from deleting the gl texture. By removing this call an application can set the cursor to A, change the cursor to B, then change the cursor back to A without having the texture deleted. 2) The temporary allocation of This->glDescription.textureName in surface.c This change should allow SetCursorProperties to retrieve "cursor A" the second time in the "A, B, A" sequence. Without this change, SetCursorProperties gets back a texture of "0" from the IWineD3DSurface_PreLoad call. This allocation is temporary since SetCursorProperties immediately resets that value: This->cursorTexture = pSur->glDescription.textureName; ... pSur->glDescription.textureName = 0; /* Prevent the texture from being changed or deleted */ I don't think I'm missing anything here... However, the implementation of item #2 could be changed around by using flags, as you discussed in a previous email, and this would seem more consistent with the intent of SFLAG_FORCELOAD (see the attached patch). Erich Hoover ehoover(a)mines.edu On 3/10/07, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
The API documentation doesn't say anything about an application releasing the d3d surface, There is a test for that somewhere in dlls/d3d9/tests/device.c (or some other test file in there).
but the documentation in the code seems to indicate that the gl texture needs to remain when the surface is deleted - I did nothing to change this. But IWineD3DSurfaceImpl_Release will destroy the gl texture of a surface when the surface is destroyed.
Also, if the gl surface still knows the texture, the following could cause an issue:
SetCursorProperties(cursorSurface, ...);
<render some stuff>
LockRect(cursorSurface, ...); <write some thing into the surface> UnlockRect(cursorSurface); PreLoad(cursorSurface);
The PreLoad will then change the cursor without SetCursorProperties beeing called. This should not happen. I tested that with some hacky test. Unfortunately this cannot be automated in the visual tests because cursors do not show up in screenshots.
This is my hacky cursor test app: http://stud4.tuwien.ac.at/~e0526822/visual.c
The successor of that file without the cursor tests is now in dlls/d3d9/tests/visual.c. I think this visual.c version is not the version I used for checking later surface changes.
Am Sonntag 11 März 2007 03:08 schrieb Erich Hoover:
I ran all of the d3d9 tests and did not get any errors with my patch applied. I did get an error when I ran your modified visual.c test, but I get the same errors with and without my patch. Yes; The existing d3d9 tests do not check what happens when the surface is actually modified, and they do not draw an actual cursor, because the device test just checks return values and not the actual drawing. But the test shows that the surface can be destroyed without modifying the cursor.
I can try to extend the test cases for modifying the surface if you want to?
On 11/03/07, Erich Hoover <ehoover(a)mines.edu> wrote:
So, which change exactly are you concerned about? Changes are: 1) The removal of "glDeleteTextures(1, &This->cursorTexture);" in device.c This change should just keep SetCursorProperties from deleting the gl texture. By removing this call an application can set the cursor to A, change the cursor to B, then change the cursor back to A without having the texture deleted. 2) The temporary allocation of This->glDescription.textureName in surface.c This change should allow SetCursorProperties to retrieve "cursor A" the second time in the "A, B, A" sequence. Without this change, SetCursorProperties gets back a texture of "0" from the IWineD3DSurface_PreLoad call. This allocation is temporary since SetCursorProperties immediately resets that value: This->cursorTexture = pSur->glDescription.textureName; ... pSur->glDescription.textureName = 0; /* Prevent the texture from being changed or deleted */
I don't think I'm missing anything here... However, the implementation of item #2 could be changed around by using flags, as you discussed in a previous email, and this would seem more consistent with the intent of SFLAG_FORCELOAD (see the attached patch).
Erich Hoover ehoover(a)mines.edu
You're trying to solve the wrong problem :-) The problem is that when we set the textureName to 0 we essentially kick the GL texture out of the surface which is then left in a somewhat inconsistent state. Calling SetCursorProperties again with the same surface will then fail because the surface no longer has a GL texture associated with it. The proper way to fix this is to make a copy of the GL texture rather than playing tricks with the surface loading code.
Yeah, that would make more sense wouldn't it :) Please see attached patch. Erich Hoover ehoover(a)mines.edu On 3/11/07, H. Verbeet <hverbeet(a)gmail.com> wrote:
On 11/03/07, Erich Hoover <ehoover(a)mines.edu> wrote:
So, which change exactly are you concerned about? Changes are: 1) The removal of "glDeleteTextures(1, &This->cursorTexture);" in device.c This change should just keep SetCursorProperties from deleting the gl texture. By removing this call an application can set the cursor to A, change the cursor to B, then change the cursor back to A without having the texture deleted. 2) The temporary allocation of This->glDescription.textureName in surface.c This change should allow SetCursorProperties to retrieve "cursor A" the second time in the "A, B, A" sequence. Without this change, SetCursorProperties gets back a texture of "0" from the IWineD3DSurface_PreLoad call. This allocation is temporary since SetCursorProperties immediately resets that value: This->cursorTexture = pSur-> glDescription.textureName; ... pSur->glDescription.textureName = 0; /* Prevent the texture from being changed or deleted */
I don't think I'm missing anything here... However, the implementation of item #2 could be changed around by using flags, as you discussed in a previous email, and this would seem more consistent with the intent of SFLAG_FORCELOAD (see the attached patch).
Erich Hoover ehoover(a)mines.edu
You're trying to solve the wrong problem :-) The problem is that when we set the textureName to 0 we essentially kick the GL texture out of the surface which is then left in a somewhat inconsistent state. Calling SetCursorProperties again with the same surface will then fail because the surface no longer has a GL texture associated with it. The proper way to fix this is to make a copy of the GL texture rather than playing tricks with the surface loading code.
Am Sonntag 11 März 2007 21:08 schrieb Erich Hoover:
Yeah, that would make more sense wouldn't it :) Please see attached patch. If you do it that way, you can remove the PreLoad, LockRect the surface(with WINED3DLOCK_READONLY), use glTexImage2D to load This->cursorTexture and then Unlock the surface. This saves uploading -> downloading -> uploading. If you use that way, please remove SFLAG_FORCELOAD too.
You should make sure that a correct gl texture unit is activated before loading This->cursorTexture with GL_EXTCALL(glActiveTextureARB(X)); Search through the code how that is done, you have to make sure that glActiveTextureARB is supported before using it. You don't have to enable GL_TEXTURE_2D to my knowledge to call glTexImage2D, but you have to restore the originally bound texture or dirtify the sampler you modify(IWineD3DDeviceImpl_MarkStateDirty(This, SAMPLER(X)); where X is the unit you selected before. (X = 0 is prefered).
Is the attached what you mean? Erich Hoover ehoover(a)mines.edu On 3/11/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Am Sonntag 11 März 2007 21:08 schrieb Erich Hoover:
Yeah, that would make more sense wouldn't it :) Please see attached patch. If you do it that way, you can remove the PreLoad, LockRect the surface(with WINED3DLOCK_READONLY), use glTexImage2D to load This->cursorTexture and then Unlock the surface. This saves uploading -> downloading -> uploading. If you use that way, please remove SFLAG_FORCELOAD too.
You should make sure that a correct gl texture unit is activated before loading This->cursorTexture with GL_EXTCALL(glActiveTextureARB(X)); Search through the code how that is done, you have to make sure that glActiveTextureARB is supported before using it. You don't have to enable GL_TEXTURE_2D to my knowledge to call glTexImage2D, but you have to restore the originally bound texture or dirtify the sampler you modify(IWineD3DDeviceImpl_MarkStateDirty(This, SAMPLER(X)); where X is the unit you selected before. (X = 0 is prefered).
On 11/03/07, Erich Hoover <ehoover(a)mines.edu> wrote:
Is the attached what you mean?
Erich Hoover ehoover(a)mines.edu
+ if (IWineD3DSurface_LockRect(pCursorBitmap, &rect, NULL, WINED3DLOCK_READONLY) == WINED3D_OK) I know the current code doesn't do it everywhere either, but you should probably be using SUCCEEDED() there.
+ void *mem = rect.pBits; ... + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0, format, type, mem); That may work, but it's not guaranteed to, see http://msdn2.microsoft.com/en-us/library/bb206357.aspx
Please examine the attached corrections. Erich Hoover ehoover(a)mines.edu On 3/11/07, H. Verbeet <hverbeet(a)gmail.com> wrote:
On 11/03/07, Erich Hoover <ehoover(a)mines.edu> wrote:
Is the attached what you mean?
Erich Hoover ehoover(a)mines.edu
+ if (IWineD3DSurface_LockRect(pCursorBitmap, &rect, NULL, WINED3DLOCK_READONLY) == WINED3D_OK) I know the current code doesn't do it everywhere either, but you should probably be using SUCCEEDED() there.
+ void *mem = rect.pBits; ... + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0, format, type, mem); That may work, but it's not guaranteed to, see http://msdn2.microsoft.com/en-us/library/bb206357.aspx
Am Sonntag 11 März 2007 23:58 schrieb Erich Hoover:
Is the attached what you mean? Yes, that looks better. A few minor things:
You can remove the SFLAG_FORCELOAD definition from wined3d_private.h now. Also, if I remember right, the cursor must be A8R8G8B8, so instead of reading the description from the surface you can read it from the pixel format table(getFormatDescEntry). I think pSur->glDescription is set on the first PreLoad only. Instead of hardcoding GL_RGBA in the glTexImage2D call use the internal format returned by getFormatDescEntry. Finally, pSur->resource.wineD3DDevice == This, so you don't have to read the device from the surface.
Erich Hoover ehoover(a)mines.edu
On 3/11/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Am Sonntag 11 März 2007 21:08 schrieb Erich Hoover:
Yeah, that would make more sense wouldn't it :) Please see attached
patch. If you do it that way, you can remove the PreLoad, LockRect the surface(with WINED3DLOCK_READONLY), use glTexImage2D to load This->cursorTexture and then Unlock the surface. This saves uploading -> downloading -> uploading. If you use that way, please remove SFLAG_FORCELOAD too.
You should make sure that a correct gl texture unit is activated before loading This->cursorTexture with GL_EXTCALL(glActiveTextureARB(X)); Search through the code how that is done, you have to make sure that glActiveTextureARB is supported before using it. You don't have to enable GL_TEXTURE_2D to my knowledge to call glTexImage2D, but you have to restore the originally bound texture or dirtify the sampler you modify(IWineD3DDeviceImpl_MarkStateDirty(This, SAMPLER(X)); where X is the unit you selected before. (X = 0 is prefered).
On 12/03/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Am Sonntag 11 März 2007 23:58 schrieb Erich Hoover: You can remove the SFLAG_FORCELOAD definition from wined3d_private.h now. You'll need to remove that from surface.c as well then.
participants (4)
-
Erich Hoover -
H. Verbeet -
Stefan Dösinger -
Stefan Dösinger