ENTER_GL and ActivateContext question

[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

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?

[Stefan Dösinger]

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

[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.

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

[Stefan Dösinger]

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.

[Alexander Dorofeyev]

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?

[Stefan Dösinger]

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.

[Ivan Gyurdiev]

> 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

[Stefan Dösinger]

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).

[Michael Stefaniuc]

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

[Stefan Dösinger]

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.

[Alexander Dorofeyev]

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 
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) {

[Stefan Dösinger]

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.

[Alexander Dorofeyev]

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.

[Roderick Colenbrander]

> 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