ENTER_GL and ActivateContext question
Hello. I was recently experimenting with a fix that involved ActivateContext and needed to figure out whether it is ok if ActivateContext is called inside ENTER_GL()... LEAVE_GL() (also if recursive ENTER_GL is ok). At first I tried to look for example code in wined3d/surface.c, which seemed to indicate this is ok, because it obviously seems to happen in one or few places. Then I found this http://bugs.winehq.org/show_bug.cgi?id=9810 from which it seems it is sort of a bug(?). Can any of 3d guys here clarify this matter - how bad is ActivateContext between ENTER_GL/LEAVE_GL and recursive ENTER_GL, does it definitely need to be avoided and should things like this ENTER_GL(); ... IWineD3DSurface_BindTexture(); ... LEAVE_GL(); (BindTexture does ActivateContext and ENTER_GL) be changed to something like ENTER_GL() ... LEAVE_GL() IWineD3DSurface_BindTexture(); ENTER_GL() ... LEAVE_GL() and submitted as patch if I find it?
Am Montag, 10. Dezember 2007 18:39:49 schrieb Alexander Dorofeyev:
Hello.
I was recently experimenting with a fix that involved ActivateContext and needed to figure out whether it is ok if ActivateContext is called inside ENTER_GL()... LEAVE_GL() (also if recursive ENTER_GL is ok). At first I tried to look for example code in wined3d/surface.c, which seemed to indicate this is ok, because it obviously seems to happen in one or few places. Then I found this ActivateContext inside ENTER_GL() and LEAVE_GL() is *not* ok, but it happens in quite a few places unfortunately. The problem this causes is stated in this bugreport. ActivateContext can call GDI calls, and they can deadlock if called between ENTER_GL and LEAVE_GL.
The example with BindTexture you gave is the main problem indeed, patches like that are a way forward. However, instead of just adding a LEAVE_GL() and ENTER_GL() it should be checked if this can be avoided by moving code around a bit. Be warned that there are a few other troubles with ActivateContext and gl calls as well, for example fbo setup vs ActivateContext
Ok, thank you and Roderick for useful info. I figure that my previous patch wasn't ideal, unfortunately it introduced one more of such recursive ENTERGL / ActivateContext situations. I'll try to improve that and send another patch. Regarding moving code around - I wish I was so competent in Direct3D and OpenGL to freely move code around in wined3d etc :). Hopefully, wine hacking may improve my grasp on it, but at the moment that's probably over my head. If you don't object, I'll do basic stuff, like putting additional LEAVE_GL/ENTER_GL in my own recent change to _BltOverride and wherever else I find similar problem, and maybe I'll look into possible redundance of glDrawBuffer call Roderick mentioned. Also may be worth to think if it's reasonable to put a substantial amount of effort into eliminating a few redundant gl* calls or a LEAVE_GL/ENTER_GL block now (then possibly also hunting regressions), when there are major problems like crashes etc. Say, to use Aliens vs Predator 2 game as an illustration - despite now one can at least get into the game proper (i.e. load a level), it's still largerly unusable because of a dinput bug and terrible performance (some fog-related software emulation fixme - I'll probably submit this to bugzilla). Stefan Dösinger wrote:
Am Montag, 10. Dezember 2007 18:39:49 schrieb Alexander Dorofeyev:
Hello.
I was recently experimenting with a fix that involved ActivateContext and needed to figure out whether it is ok if ActivateContext is called inside ENTER_GL()... LEAVE_GL() (also if recursive ENTER_GL is ok). At first I tried to look for example code in wined3d/surface.c, which seemed to indicate this is ok, because it obviously seems to happen in one or few places. Then I found this ActivateContext inside ENTER_GL() and LEAVE_GL() is *not* ok, but it happens in quite a few places unfortunately. The problem this causes is stated in this bugreport. ActivateContext can call GDI calls, and they can deadlock if called between ENTER_GL and LEAVE_GL.
The example with BindTexture you gave is the main problem indeed, patches like that are a way forward. However, instead of just adding a LEAVE_GL() and ENTER_GL() it should be checked if this can be avoided by moving code around a bit. Be warned that there are a few other troubles with ActivateContext and gl calls as well, for example fbo setup vs ActivateContext
Am Dienstag, 11. Dezember 2007 07:06:02 schrieb Alexander Dorofeyev:
Ok, thank you and Roderick for useful info. I figure that my previous patch wasn't ideal, unfortunately it introduced one more of such recursive ENTERGL / ActivateContext situations. I'll try to improve that and send another patch.
Regarding moving code around - I wish I was so competent in Direct3D and OpenGL to freely move code around in wined3d etc :). Hopefully, wine hacking may improve my grasp on it, but at the moment that's probably over my head. If you don't object, I'll do basic stuff, like putting additional LEAVE_GL/ENTER_GL in my own recent change to _BltOverride and wherever else I find similar problem, and maybe I'll look into possible redundance of glDrawBuffer call Roderick mentioned. We definitely need some help in cleaning up issues like that, so help is highly welcome! Quite often it is the "basic" stuff that is annoying and time consuming.
Hello. I've spent some time trying to get into this stuff and what can be improved in IWineD3DSurfaceImpl_BltOverride. Code that does glDrawBuffer in the Colorfill path and possibly elsewhere indeed appears to be redundant to ActivateContext and probably should go. That's in theory :), in practice though I've run into some problems. IWineD3DDevice_Clear call that it uses to perform the job does a thing that looks strange to me: ActivateContext(This, This->render_targets[0], CTXUSAGE_CLEAR); So whatever the IWineD3DSurfaceImpl_BltOverride does to properly ActivateContext select draw buffer etc may be overriden by ActivateContext within IWineD3DDevice_Clear seemingly. This looks buggy(?). Often things still get drawn properly because ActivateContext within IWineD3DDevice_Clear in many cases doesn't see a need to change context or do glDrawBuffer, so nothing happens even though it's passed not that render target which should get cleared. My recent change to BltOverride though made problems more likely, in fact it appears it broke something as basic as ddraw colorfill to the front buffer / primary surface if a back buffer exists :(, because now in such case ActivateContext called from Clear with wrong target is guaranteed to do its job and back buffer gets cleared instead. Do you have any advices on how to handle that? In my tests I tried to use Get/SetRenderTarget before calling Clear, and restore the old one afterwards, which seems to make the colorfill work right on front buffer, but maybe there are better/less overhead ways to force Clear to pass right render target to ActivateContext? Or maybe something along the lines of device->isInDraw mechanism I saw in few other places, to tell Clear not to bother with ActivateContext at all?
Hi, You are right, there is some problem here. I think that the code originates from times when this codepath in colorfill was only entered when iface == device->render_targets[0], or when Clear didn't set any draw buffer itself. But you have spotted correctly, if iface is the device's front buffer, then the colorfill will fail. device-isInDraw is not correct for this, because Clear() can never be called during a draw; DrawPrimitive doesn't call it, and another thread can't call it because the critical section is held. Installing a similar variable is possible, but hacky. Another way would be to reverse the mapping, and make Clear() call colorfill; That would run through the whole find-the-surface code however, and be more overhead than a SetRenderTarget; Furthermore it is un-d3d9ish. Also, Clear() takes a few parameters into account that colorfill doesn't GetRenderTarget and SetRenderTarget is a useable way to solve this. SetRenderTarget currently calls ActivateContext itself, but this call should be removed. I added it during the context management introduction to lower the propability of regressions. A lot of code in WineD3D avoids the getters and setters, and just accesses the implementation structure. While any object oriented programmer screams when doing that, we need it for performance reasons. Currently we spend 10 to 20 per cent of the CPU time spent in WineD3D just in surface::GetContainer and its callees, if we'd use Getters and Setters everywhere then goodbye performance(I want to replace GetContainer somewhen). However, GetRenderTarget and SetRenderTarget do a bit of setup and refcounting, so bypassing that certainly has an impact on maintainability. I'd be fine with direct impl access in this case, but it is a corner case. A last option is to move the biggest part of Clear() into a subroutine, call the ActivateContext in Clear and BltOverride, and perform the rest of the job in the subroutine. The drawback of this is that d3d8 and d3d9 clears run through an extra call, but it avoids the SetRenderTarget/GetRenderTarget overhead and direct implementation access. The function could be inlined as well, but I am not sure if that works across .c files.
A lot of code in WineD3D avoids the getters and setters, and just accesses the implementation structure. While any object oriented programmer screams when doing that, we need it for performance reasons. Currently we spend 10 to 20 per cent of the CPU time spent in WineD3D just in surface::GetContainer and its callees, if we'd use Getters and Setters everywhere then goodbye performance(I want to replace GetContainer somewhen).
So inline it... imho the performance argument is a bad reason to abandon maintainability. Your codebase is constantly getting more complex, and now you're having threading issues, which will only get more important with multi-core and other such things becoming standard. Meanwhile graphics cards are getting a lot more powerful - I solved the performance problem 2 weeks ago w/ a GeForce 8800 GT. As for the "object oriented programmer" - I would hope every programmer thinks in terms of objects, even when writing C code. The GLSL backend wouldn't be available today if the shader compiler was still a big series of if statements. Ivan
Am Freitag, 14. Dezember 2007 02:46:42 schrieb Ivan Gyurdiev:
Your codebase is constantly getting more complex, and now you're having threading issues, which will only get more important with multi-core and other such things becoming standard. Meanwhile graphics cards are getting a lot more powerful - I solved the performance problem 2 weeks ago w/ a GeForce 8800 GT. For every other program I'd agree with that, but for WineD3D that doesn't apply. Gamers do not care about algorithmic complexity and the the-next-gen-hardware-will-fix-it argument. This is true for almost everything else out there, but gamers see that they get 70 fps in windows and 45 in Wine, and they'll complain. Heck, they even complain if they get 70 in Windows and 65 in Wine.
As for the "object oriented programmer" - I would hope every programmer thinks in terms of objects, even when writing C code. The GLSL backend wouldn't be available today if the shader compiler was still a big series of if statements. This is true; And the rest of the code isn't a collection of if statements either(BltOverride is, but that should be fixed). If we had proper C++, then we could have the compiler inline the getters and setters, however, with the current C setup we can't do that(unless we write a bunch of macros or extra inline functions, which doesn't really help maintainability either).
Stefan Dösinger wrote:
Am Freitag, 14. Dezember 2007 02:46:42 schrieb Ivan Gyurdiev:
As for the "object oriented programmer" - I would hope every programmer thinks in terms of objects, even when writing C code. The GLSL backend wouldn't be available today if the shader compiler was still a big series of if statements. This is true; And the rest of the code isn't a collection of if statements either(BltOverride is, but that should be fixed). If we had proper C++, then we could have the compiler inline the getters and setters, however, with the current C setup we can't do that(unless we write a bunch of macros or extra inline functions, which doesn't really help maintainability either). I don't know what you mean with proper inline. It's true that "inline" is only a hint to the compiler but normally the compiler knows anyway better what to inline and what not.
If you really want to force an inline you have the __attribute__((always_inline)) bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 Consulting Communications Engineer Fax.: +49-711-96437-111 -------------------------------------------------------------------- Reg. Adresse: Red Hat GmbH, Hauptstätter Strasse 58, 70178 Stuttgart Handelsregister: Amtsgericht Stuttgart HRB 153243 Geschäftsführer: Brendan Lane, Charlie Peters, Michael Cunningham, Werner Knoblich
Am Freitag, 14. Dezember 2007 10:55:10 schrieb Michael Stefaniuc:
I don't know what you mean with proper inline. It's true that "inline" is only a hint to the compiler but normally the compiler knows anyway better what to inline and what not.
If you really want to force an inline you have the __attribute__((always_inline)) What I meant is that we can do a method call, and the C++ compiler finds out statically at compile time where it will go, and inlines the method implementation. For example, we do IWineD3DSurface::GetContainer, and the compiler knows that we have an instance of IWineD3DSurface, and the implementation of GetContainer is IWineD3DBaseSurface::GetContainer, and inlines it.
Hi. Here's preliminary version of a changes to BltOverride/Clear, with Clear subroutine approach discussed previously. Please look through it if you can. Basically everything except a check for error condition and deciding the target surface moved from Clear to subroutine. Another thing I did is moving code that does IWineD3DSurface_LoadLocation below the ActivateContext in the subroutine (originally it was done prior to ActivateContext), because it seems to do GL calls at least in some codepaths and doesn't seem to do ActivateContext. Another issue I noticed in tests (seems to be present in wine-0.9.50 too, before ActivateContext was added to BltOverride) is that BltOverride/colorfill to front buffer often doesn't actually make filled rectangles appear - they can fail to appear for indefinite time, especially if there are no subsequent identical or similar calls, but there are no visible errors in traces. glFlush does make them appear, so maybe they are somehow getting buffered. This seems to be specific to front buffer. Maybe glFlush really should be called in BltOverride (or clearing subroutine) if the target is front buffer? Stefan Dösinger wrote:
A last option is to move the biggest part of Clear() into a subroutine, call the ActivateContext in Clear and BltOverride, and perform the rest of the job in the subroutine. The drawback of this is that d3d8 and d3d9 clears run through an extra call, but it avoids the SetRenderTarget/GetRenderTarget overhead and direct implementation access. The function could be inlined as well, but I am not sure if that works across .c files.
From 017f0600dccb3fd99af61b941676a6aa2b18d733 Mon Sep 17 00:00:00 2001 From: Alexander Dorofeyev <alexd4(a)inbox.lv> Date: Sat, 15 Dec 2007 01:56:44 -0800 Subject: [PATCH] wined3d: restructure and cleanup colorfill codepath in IWineD3DSurfaceImpl_BltOverride
--- dlls/wined3d/device.c | 160 ++++++++++++++++++++++++++++++++++++++++ dlls/wined3d/surface.c | 50 ++++--------- dlls/wined3d/wined3d_private.h | 3 + 3 files changed, 177 insertions(+), 36 deletions(-) diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index 8245563..bafa740 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -4780,6 +4780,166 @@ static HRESULT WINAPI IWineD3DDeviceImpl_Present(IWineD3DDevice *iface, return WINED3D_OK; } +/* Not called from the VTable (internal subroutine) */ +void IWineD3DDeviceImpl_ClearSurface(IWineD3DDeviceImpl *This, IWineD3DSurfaceImpl *target, DWORD Count, + CONST WINED3DRECT* pRects, DWORD Flags, WINED3DCOLOR Color, + float Z, DWORD Stencil) { + GLbitfield glMask = 0; + unsigned int i; + WINED3DRECT curRect; + RECT vp_rect; + WINED3DVIEWPORT *vp = &This->stateBlock->viewport; + UINT drawable_width, drawable_height; + + ActivateContext(This, (IWineD3DSurface *) target, CTXUSAGE_CLEAR); + /* When we're clearing parts of the drawable, make sure that the target surface is well up to date in the + * drawable. After the clear we'll mark the drawable up to date, so we have to make sure that this is true + * for the cleared parts, and the untouched parts. + * + * If we're clearing the whole target there is no need to copy it into the drawable, it will be overwritten + * anyway. If we're not clearing the color buffer we don't have to copy either since we're not going to set + * the drawable up to date. We have to check all settings that limit the clear area though. Do not bother + * checking all this if the dest surface is in the drawable anyway. + */ + if((Flags & WINED3DCLEAR_TARGET) && !(target->Flags & SFLAG_INDRAWABLE)) { + while(1) { + if(vp->X != 0 || vp->Y != 0 || + vp->Width < target->currentDesc.Width || vp->Height < target->currentDesc.Height) { + IWineD3DSurface_LoadLocation((IWineD3DSurface *) target, SFLAG_INDRAWABLE, NULL); + break; + } + if(This->stateBlock->renderState[WINED3DRS_SCISSORTESTENABLE] && ( + This->stateBlock->scissorRect.left > 0 || This->stateBlock->scissorRect.top > 0 || + This->stateBlock->scissorRect.right < target->currentDesc.Width || + This->stateBlock->scissorRect.bottom < target->currentDesc.Height)) { + IWineD3DSurface_LoadLocation((IWineD3DSurface *) target, SFLAG_INDRAWABLE, NULL); + break; + } + if(Count > 0 && pRects && ( + pRects[0].x1 > 0 || pRects[0].y1 > 0 || + pRects[0].x2 < target->currentDesc.Width || + pRects[0].y2 < target->currentDesc.Height)) { + IWineD3DSurface_LoadLocation((IWineD3DSurface *) target, SFLAG_INDRAWABLE, NULL); + break; + } + break; + } + } + + target->get_drawable_size(target, &drawable_width, &drawable_height); + + ENTER_GL(); + + if (wined3d_settings.offscreen_rendering_mode == ORM_FBO) { + apply_fbo_state((IWineD3DDevice *) This); + } + + /* Only set the values up once, as they are not changing */ + if (Flags & WINED3DCLEAR_STENCIL) { + glClearStencil(Stencil); + checkGLcall("glClearStencil"); + glMask = glMask | GL_STENCIL_BUFFER_BIT; + glStencilMask(0xFFFFFFFF); + } + + if (Flags & WINED3DCLEAR_ZBUFFER) { + glDepthMask(GL_TRUE); + glClearDepth(Z); + checkGLcall("glClearDepth"); + glMask = glMask | GL_DEPTH_BUFFER_BIT; + IWineD3DDeviceImpl_MarkStateDirty(This, STATE_RENDER(WINED3DRS_ZWRITEENABLE)); + } + + if (Flags & WINED3DCLEAR_TARGET) { + TRACE("Clearing screen with glClear to color %x\n", Color); + glClearColor(D3DCOLOR_R(Color), + D3DCOLOR_G(Color), + D3DCOLOR_B(Color), + D3DCOLOR_A(Color)); + checkGLcall("glClearColor"); + + /* Clear ALL colors! */ + glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + glMask = glMask | GL_COLOR_BUFFER_BIT; + } + + vp_rect.left = vp->X; + vp_rect.top = vp->Y; + vp_rect.right = vp->X + vp->Width; + vp_rect.bottom = vp->Y + vp->Height; + if (!(Count > 0 && pRects)) { + if(This->stateBlock->renderState[WINED3DRS_SCISSORTESTENABLE]) { + IntersectRect(&vp_rect, &vp_rect, &This->stateBlock->scissorRect); + } + if(This->render_offscreen) { + glScissor(vp_rect.left, vp_rect.top, + vp_rect.right - vp_rect.left, vp_rect.bottom - vp_rect.top); + } else { + glScissor(vp_rect.left, drawable_height - vp_rect.bottom, + vp_rect.right - vp_rect.left, vp_rect.bottom - vp_rect.top); + } + checkGLcall("glScissor"); + glClear(glMask); + checkGLcall("glClear"); + } else { + /* Now process each rect in turn */ + for (i = 0; i < Count; i++) { + /* Note gl uses lower left, width/height */ + IntersectRect((RECT *) &curRect, &vp_rect, (RECT *) &pRects[i]); + if(This->stateBlock->renderState[WINED3DRS_SCISSORTESTENABLE]) { + IntersectRect((RECT *) &curRect, (RECT *) &curRect, &This->stateBlock->scissorRect); + } + TRACE("(%p) Rect=(%d,%d)->(%d,%d) glRect=(%d,%d), len=%d, hei=%d\n", This, + pRects[i].x1, pRects[i].y1, pRects[i].x2, pRects[i].y2, + curRect.x1, (target->currentDesc.Height - curRect.y2), + curRect.x2 - curRect.x1, curRect.y2 - curRect.y1); + + /* Tests show that rectangles where x1 > x2 or y1 > y2 are ignored silently. + * The rectangle is not cleared, no error is returned, but further rectanlges are + * still cleared if they are valid + */ + if(curRect.x1 > curRect.x2 || curRect.y1 > curRect.y2) { + TRACE("Rectangle with negative dimensions, ignoring\n"); + continue; + } + + if(This->render_offscreen) { + glScissor(curRect.x1, curRect.y1, + curRect.x2 - curRect.x1, curRect.y2 - curRect.y1); + } else { + glScissor(curRect.x1, drawable_height - curRect.y2, + curRect.x2 - curRect.x1, curRect.y2 - curRect.y1); + } + checkGLcall("glScissor"); + + glClear(glMask); + checkGLcall("glClear"); + } + } + + /* Restore the old values (why..?) */ + if (Flags & WINED3DCLEAR_STENCIL) { + glStencilMask(This->stateBlock->renderState[WINED3DRS_STENCILWRITEMASK]); + } + if (Flags & WINED3DCLEAR_TARGET) { + DWORD mask = This->stateBlock->renderState[WINED3DRS_COLORWRITEENABLE]; + glColorMask(mask & WINED3DCOLORWRITEENABLE_RED ? GL_TRUE : GL_FALSE, + mask & WINED3DCOLORWRITEENABLE_GREEN ? GL_TRUE : GL_FALSE, + mask & WINED3DCOLORWRITEENABLE_BLUE ? GL_TRUE : GL_FALSE, + mask & WINED3DCOLORWRITEENABLE_ALPHA ? GL_TRUE : GL_FALSE); + + /* Dirtify the target surface for now. If the surface is locked regularly, and an up to date sysmem copy exists, + * it is most likely more efficient to perform a clear on the sysmem copy too instead of downloading it + */ + IWineD3DSurface_ModifyLocation(This->lastActiveRenderTarget, SFLAG_INDRAWABLE, TRUE); + /* TODO: Move the fbo logic into ModifyLocation() */ + if(This->render_offscreen && wined3d_settings.offscreen_rendering_mode == ORM_FBO) { + target->Flags |= SFLAG_INTEXTURE; + } + } + LEAVE_GL(); +} + static HRESULT WINAPI IWineD3DDeviceImpl_Clear(IWineD3DDevice *iface, DWORD Count, CONST WINED3DRECT* pRects, DWORD Flags, WINED3DCOLOR Color, float Z, DWORD Stencil) { IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *)iface; diff --git a/dlls/wined3d/surface.c b/dlls/wined3d/surface.c index 3f69baf..ab8749b 100644 --- a/dlls/wined3d/surface.c +++ b/dlls/wined3d/surface.c @@ -3201,6 +3201,15 @@ static HRESULT IWineD3DSurfaceImpl_BltOverride(IWineD3DSurfaceImpl *This, RECT * TRACE("Colorfill\n"); + /* This == (IWineD3DSurfaceImpl *) myDevice->render_targets[0] || dstSwapchain + must be true if we are here */ + if (This != (IWineD3DSurfaceImpl *) myDevice->render_targets[0] && + !(This == (IWineD3DSurfaceImpl*) dstSwapchain->frontBuffer || + (dstSwapchain->backBuffer && This == (IWineD3DSurfaceImpl*) dstSwapchain->backBuffer[0]))) { + TRACE("Surface is higher back buffer, falling back to software\n"); + return WINED3DERR_INVALIDCALL; + } + /* The color as given in the Blt function is in the format of the frame-buffer... * 'clear' expect it in ARGB format => we need to do some conversion :-) */ @@ -3236,43 +3245,12 @@ static HRESULT IWineD3DSurfaceImpl_BltOverride(IWineD3DSurfaceImpl *This, RECT * return WINED3DERR_INVALIDCALL; } - ActivateContext(myDevice, (IWineD3DSurface *) This, CTXUSAGE_RESOURCELOAD); - ENTER_GL(); - - if(dstSwapchain && dstSwapchain->backBuffer && This == (IWineD3DSurfaceImpl*) dstSwapchain->backBuffer[0]) { - glDrawBuffer(GL_BACK); - checkGLcall("glDrawBuffer(GL_BACK)"); - } else if (dstSwapchain && This == (IWineD3DSurfaceImpl*) dstSwapchain->frontBuffer) { - glDrawBuffer(GL_FRONT); - checkGLcall("glDrawBuffer(GL_FRONT)"); - } else if(This == (IWineD3DSurfaceImpl *) myDevice->render_targets[0]) { - glDrawBuffer(myDevice->offscreenBuffer); - checkGLcall("glDrawBuffer(myDevice->offscreenBuffer3)"); - } else { - LEAVE_GL(); - TRACE("Surface is higher back buffer, falling back to software\n"); - return WINED3DERR_INVALIDCALL; - } - TRACE("(%p) executing Render Target override, color = %x\n", This, color); - - IWineD3DDevice_Clear( (IWineD3DDevice *) myDevice, - 1 /* Number of rectangles */, - &rect, - WINED3DCLEAR_TARGET, - color, - 0.0 /* Z */, - 0 /* Stencil */); - - /* Restore the original draw buffer */ - if(!dstSwapchain) { - glDrawBuffer(myDevice->offscreenBuffer); - } else if(dstSwapchain->backBuffer && dstSwapchain->backBuffer[0]) { - glDrawBuffer(GL_BACK); - } - vcheckGLcall("glDrawBuffer"); - LEAVE_GL(); - + IWineD3DDeviceImpl_ClearSurface(myDevice, This, + 1, /* Number of rectangles */ + &rect, WINED3DCLEAR_TARGET, color, + 0.0 /* Z */, + 0 /* Stencil */); return WINED3D_OK; } } diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 6806bfe..7951b9b 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -773,6 +773,9 @@ struct IWineD3DDeviceImpl extern const IWineD3DDeviceVtbl IWineD3DDevice_Vtbl; +void IWineD3DDeviceImpl_ClearSurface(IWineD3DDeviceImpl *This, IWineD3DSurfaceImpl *target, DWORD Count, + CONST WINED3DRECT* pRects, DWORD Flags, WINED3DCOLOR Color, + float Z, DWORD Stencil); void IWineD3DDeviceImpl_FindTexUnitMap(IWineD3DDeviceImpl *This); void IWineD3DDeviceImpl_MarkStateDirty(IWineD3DDeviceImpl *This, DWORD state); static inline BOOL isStateDirty(WineD3DContext *context, DWORD state) { -- 1.5.3.4
From 5e62fefe87efbac9620aa65e00ac67fea502ad95 Mon Sep 17 00:00:00 2001 From: Alexander Dorofeyev <alexd4(a)inbox.lv> Date: Sat, 15 Dec 2007 01:58:27 -0800 Subject: [PATCH] wined3d: restructure IWineD3DDeviceImpl_Clear
--- dlls/wined3d/device.c | 158 +------------------------------------------------ 1 files changed, 1 insertions(+), 157 deletions(-) diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index bafa740..1bcd412 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -4945,13 +4945,6 @@ static HRESULT WINAPI IWineD3DDeviceImpl_Clear(IWineD3DDevice *iface, DWORD Coun IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *)iface; IWineD3DSurfaceImpl *target = (IWineD3DSurfaceImpl *)This->render_targets[0]; - GLbitfield glMask = 0; - unsigned int i; - WINED3DRECT curRect; - RECT vp_rect; - WINED3DVIEWPORT *vp = &This->stateBlock->viewport; - UINT drawable_width, drawable_height; - TRACE("(%p) Count (%d), pRects (%p), Flags (%x), Color (0x%08x), Z (%f), Stencil (%d)\n", This, Count, pRects, Flags, Color, Z, Stencil); @@ -4961,156 +4954,7 @@ static HRESULT WINAPI IWineD3DDeviceImpl_Clear(IWineD3DDevice *iface, DWORD Coun return WINED3DERR_INVALIDCALL; } - /* When we're clearing parts of the drawable, make sure that the target surface is well up to date in the - * drawable. After the clear we'll mark the drawable up to date, so we have to make sure that this is true - * for the cleared parts, and the untouched parts. - * - * If we're clearing the whole target there is no need to copy it into the drawable, it will be overwritten - * anyway. If we're not clearing the color buffer we don't have to copy either since we're not going to set - * the drawable up to date. We have to check all settings that limit the clear area though. Do not bother - * checking all this if the dest surface is in the drawable anyway. - */ - if((Flags & WINED3DCLEAR_TARGET) && !(target->Flags & SFLAG_INDRAWABLE)) { - while(1) { - if(vp->X != 0 || vp->Y != 0 || - vp->Width < target->currentDesc.Width || vp->Height < target->currentDesc.Height) { - IWineD3DSurface_LoadLocation((IWineD3DSurface *) target, SFLAG_INDRAWABLE, NULL); - break; - } - if(This->stateBlock->renderState[WINED3DRS_SCISSORTESTENABLE] && ( - This->stateBlock->scissorRect.left > 0 || This->stateBlock->scissorRect.top > 0 || - This->stateBlock->scissorRect.right < target->currentDesc.Width || - This->stateBlock->scissorRect.bottom < target->currentDesc.Height)) { - IWineD3DSurface_LoadLocation((IWineD3DSurface *) target, SFLAG_INDRAWABLE, NULL); - break; - } - if(Count > 0 && pRects && ( - pRects[0].x1 > 0 || pRects[0].y1 > 0 || - pRects[0].x2 < target->currentDesc.Width || - pRects[0].y2 < target->currentDesc.Height)) { - IWineD3DSurface_LoadLocation((IWineD3DSurface *) target, SFLAG_INDRAWABLE, NULL); - break; - } - break; - } - } - - target->get_drawable_size(target, &drawable_width, &drawable_height); - - /* This is for offscreen rendering as well as for multithreading, thus activate the set render target - * and not the last active one. - */ - ActivateContext(This, This->render_targets[0], CTXUSAGE_CLEAR); - ENTER_GL(); - - if (wined3d_settings.offscreen_rendering_mode == ORM_FBO) { - apply_fbo_state(iface); - } - - /* Only set the values up once, as they are not changing */ - if (Flags & WINED3DCLEAR_STENCIL) { - glClearStencil(Stencil); - checkGLcall("glClearStencil"); - glMask = glMask | GL_STENCIL_BUFFER_BIT; - glStencilMask(0xFFFFFFFF); - } - - if (Flags & WINED3DCLEAR_ZBUFFER) { - glDepthMask(GL_TRUE); - glClearDepth(Z); - checkGLcall("glClearDepth"); - glMask = glMask | GL_DEPTH_BUFFER_BIT; - IWineD3DDeviceImpl_MarkStateDirty(This, STATE_RENDER(WINED3DRS_ZWRITEENABLE)); - } - - if (Flags & WINED3DCLEAR_TARGET) { - TRACE("Clearing screen with glClear to color %x\n", Color); - glClearColor(D3DCOLOR_R(Color), - D3DCOLOR_G(Color), - D3DCOLOR_B(Color), - D3DCOLOR_A(Color)); - checkGLcall("glClearColor"); - - /* Clear ALL colors! */ - glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); - glMask = glMask | GL_COLOR_BUFFER_BIT; - } - - vp_rect.left = vp->X; - vp_rect.top = vp->Y; - vp_rect.right = vp->X + vp->Width; - vp_rect.bottom = vp->Y + vp->Height; - if (!(Count > 0 && pRects)) { - if(This->stateBlock->renderState[WINED3DRS_SCISSORTESTENABLE]) { - IntersectRect(&vp_rect, &vp_rect, &This->stateBlock->scissorRect); - } - if(This->render_offscreen) { - glScissor(vp_rect.left, vp_rect.top, - vp_rect.right - vp_rect.left, vp_rect.bottom - vp_rect.top); - } else { - glScissor(vp_rect.left, drawable_height - vp_rect.bottom, - vp_rect.right - vp_rect.left, vp_rect.bottom - vp_rect.top); - } - checkGLcall("glScissor"); - glClear(glMask); - checkGLcall("glClear"); - } else { - /* Now process each rect in turn */ - for (i = 0; i < Count; i++) { - /* Note gl uses lower left, width/height */ - IntersectRect((RECT *) &curRect, &vp_rect, (RECT *) &pRects[i]); - if(This->stateBlock->renderState[WINED3DRS_SCISSORTESTENABLE]) { - IntersectRect((RECT *) &curRect, (RECT *) &curRect, &This->stateBlock->scissorRect); - } - TRACE("(%p) Rect=(%d,%d)->(%d,%d) glRect=(%d,%d), len=%d, hei=%d\n", This, - pRects[i].x1, pRects[i].y1, pRects[i].x2, pRects[i].y2, - curRect.x1, (target->currentDesc.Height - curRect.y2), - curRect.x2 - curRect.x1, curRect.y2 - curRect.y1); - - /* Tests show that rectangles where x1 > x2 or y1 > y2 are ignored silently. - * The rectangle is not cleared, no error is returned, but further rectanlges are - * still cleared if they are valid - */ - if(curRect.x1 > curRect.x2 || curRect.y1 > curRect.y2) { - TRACE("Rectangle with negative dimensions, ignoring\n"); - continue; - } - - if(This->render_offscreen) { - glScissor(curRect.x1, curRect.y1, - curRect.x2 - curRect.x1, curRect.y2 - curRect.y1); - } else { - glScissor(curRect.x1, drawable_height - curRect.y2, - curRect.x2 - curRect.x1, curRect.y2 - curRect.y1); - } - checkGLcall("glScissor"); - - glClear(glMask); - checkGLcall("glClear"); - } - } - - /* Restore the old values (why..?) */ - if (Flags & WINED3DCLEAR_STENCIL) { - glStencilMask(This->stateBlock->renderState[WINED3DRS_STENCILWRITEMASK]); - } - if (Flags & WINED3DCLEAR_TARGET) { - DWORD mask = This->stateBlock->renderState[WINED3DRS_COLORWRITEENABLE]; - glColorMask(mask & WINED3DCOLORWRITEENABLE_RED ? GL_TRUE : GL_FALSE, - mask & WINED3DCOLORWRITEENABLE_GREEN ? GL_TRUE : GL_FALSE, - mask & WINED3DCOLORWRITEENABLE_BLUE ? GL_TRUE : GL_FALSE, - mask & WINED3DCOLORWRITEENABLE_ALPHA ? GL_TRUE : GL_FALSE); - - /* Dirtify the target surface for now. If the surface is locked regularly, and an up to date sysmem copy exists, - * it is most likely more efficient to perform a clear on the sysmem copy too instead of downloading it - */ - IWineD3DSurface_ModifyLocation(This->lastActiveRenderTarget, SFLAG_INDRAWABLE, TRUE); - /* TODO: Move the fbo logic into ModifyLocation() */ - if(This->render_offscreen && wined3d_settings.offscreen_rendering_mode == ORM_FBO) { - target->Flags |= SFLAG_INTEXTURE; - } - } - LEAVE_GL(); + IWineD3DDeviceImpl_ClearSurface(This, target, Count, pRects, Flags, Color, Z, Stencil); return WINED3D_OK; } -- 1.5.3.4
Am Samstag, 15. Dezember 2007 12:19:36 schrieb Alexander Dorofeyev:
This seems to be specific to front buffer. Maybe glFlush really should be called in BltOverride (or clearing subroutine) if the target is front buffer? Yes, glFlush() has to be called when writing to the front buffer. SwapBuffers does an implicit flush, so for the back buffer you don't have to do anything, but the front buffer needs a glFlush(). It isn't there yet, so it should be added.
Your patch looks quite good, but you should remove the original code from IWineD3DDeviceImpl_Clear() and make it call ClearSurface() as well. The fbo activation you moved is a different problem. It needs to be fixed as well, but it is not as easy as swapping the calls :-( . Essentially, ActivateContext has to be called before the fbo setup, due to multithreading concerns. On the other hand, some things ActivateContext does fail if an fbo is active, so fbos must be set before activatecontext. Essentially we'll have to move fbo setup into ActivateContext, but this has some implications on multiple render targets and depth stencil swapping which need more investigation. OpenGL driver limitations play a role here as well.
Hi. Stefan Dösinger wrote:
Am Samstag, 15. Dezember 2007 12:19:36 schrieb Alexander Dorofeyev:
This seems to be specific to front buffer. Maybe glFlush really should be called in BltOverride (or clearing subroutine) if the target is front buffer? Yes, glFlush() has to be called when writing to the front buffer. SwapBuffers does an implicit flush, so for the back buffer you don't have to do anything, but the front buffer needs a glFlush(). It isn't there yet, so it should be added.
OK, I'll work on that. There seem to be more places with this issue, at least UnlockRect appears to need it, it helps problems in one ddraw game I tested (Red Alert 2). BTW, if no swapchain exists for a render target and iface == device->render_targets[0], it's a front buffer, right?
The fbo activation you moved is a different problem. It needs to be fixed as well, but it is not as easy as swapping the calls :-( . Essentially, ActivateContext has to be called before the fbo setup, due to multithreading concerns. On the other hand, some things ActivateContext does fail if an fbo is active, so fbos must be set before activatecontext. Essentially we'll have to move fbo setup into ActivateContext, but this has some implications on multiple render targets and depth stencil swapping which need more investigation. OpenGL driver limitations play a role here as well.
Just to be sure, by fbo activation you mean LoadLocation(..., SFLAG_INDRAWABLE, ...) or apply_fbo_state? I think I only moved the former relative to ActivateContext. At the moment I backtracked on that change anyway. Actually I guess I was wrong that LoadLocation before ActivateContext may be a problem, because in SFLAG_INDRAWABLE case LoadLocation seems to call either surface_blt_to_drawable or flush_to_framebuffer_drawpixels both of which do ActivateContext. GL calls are in other codepaths that won't be entered, unfortunately I didn't notice it previous time. At least that's so at the moment.
Hello.
I was recently experimenting with a fix that involved ActivateContext and needed to figure out whether it is ok if ActivateContext is called inside ENTER_GL()... LEAVE_GL() (also if recursive ENTER_GL is ok). At first I tried to look for example code in wined3d/surface.c, which seemed to indicate this is ok, because it obviously seems to happen in one or few places. Then I found this
http://bugs.winehq.org/show_bug.cgi?id=9810
from which it seems it is sort of a bug(?). Can any of 3d guys here clarify this matter - how bad is ActivateContext between ENTER_GL/LEAVE_GL and recursive ENTER_GL, does it definitely need to be avoided and should things like this
ENTER_GL(); ... IWineD3DSurface_BindTexture(); ... LEAVE_GL();
(BindTexture does ActivateContext and ENTER_GL)
be changed to something like
ENTER_GL() ... LEAVE_GL()
IWineD3DSurface_BindTexture();
ENTER_GL() ... LEAVE_GL()
and submitted as patch if I find it?
GDI calls should be avoided within ENTER_GL/LEAVE_GL ('wine_tsx11_lock') as it can conflict with the GDI lock. (WGL calls used to be standard GDI calls, they are less sensitive to this now). ActivateContext is one of the calls which can make GDI calls and that's why it should be avoided. A while ago I fixed most of these cases but I haven't played with the code for lets say a month or so. This piece seems to be new. Regards, Roderick -- Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen! Ideal für Modem und ISDN: http://www.gmx.net/de/go/smartsurfer
participants (6)
-
Alexander Dorofeyev -
Ivan Gyurdiev -
Michael Stefaniuc -
Roderick Colenbrander -
Stefan Dösinger -
Stefan Dösinger