Re: [PATCH 14/15] wined3d: Add device->isInDraw checks in LoadLocation.
On Sun, Apr 6, 2008 at 5:06 PM, Alexander Dorofeyev <alexd4(a)inbox.lv> wrote:
Prevents calling ActivateContext while holding gl lock, e.g. when preloading texture in sampler().
There may be a problem here with drawPrimitive, which calls LoadLocation before ActivateContext while having isInDraw=TRUE. If the primary render target is an offscreen FBO surface, LoadLocation will follow the SFLAG_INTEXTURE path and it may end up making gl calls without a context in place. - Allan
Oh, you are right. Must be fixed some other way then, I guess, maybe moving assignment to isInDraw lower or moving ActivateContext higher. From what I understood from Bioshock bug (http://bugs.winehq.org/show_bug.cgi?id=9973), moving ActivateContext in drawPrimitive produces bugs in other programs, right? Could you add to that bug a short description of what games etc needed to reproduce these problems? Allan Tong wrote:
On Sun, Apr 6, 2008 at 5:06 PM, Alexander Dorofeyev <alexd4(a)inbox.lv> wrote:
Prevents calling ActivateContext while holding gl lock, e.g. when preloading texture in sampler().
There may be a problem here with drawPrimitive, which calls LoadLocation before ActivateContext while having isInDraw=TRUE. If the primary render target is an offscreen FBO surface, LoadLocation will follow the SFLAG_INTEXTURE path and it may end up making gl calls without a context in place.
- Allan
Am Montag, 7. April 2008 08:30:20 schrieb Alexander Dorofeyev:
Oh, you are right. Must be fixed some other way then, I guess, maybe moving assignment to isInDraw lower or moving ActivateContext higher. From what I understood from Bioshock bug (http://bugs.winehq.org/show_bug.cgi?id=9973), moving ActivateContext in drawPrimitive produces bugs in other programs, right? Could you add to that bug a short description of what games etc needed to reproduce these problems? There are two design issues here:
One is that FindContext, which executes before the wglMakeCurrent call does OpenGL calls. In some situations it has to do, e.g. reading the texture from a Pbuffer before switching to some other buffer, in some cases it can't, e.g. on a thread switch. I am not sure about the solution yet, but maybe we have to ActivateContext the old render target in FindContext before reading back textures if we find a thread switch and target switch at the same time. The other issue, and that's the problem with Bioshock, is that ActivateContext isn't really aware of the FBO stuff. I've been discussing this with Henri some time ago, but we didn't find a solution. The nicest way would be to make ActivateContext select tge fbo, color buffer 0 and the depth stencil, and to make color buffers 1+ regular states handled in the state table. Unfortunately that doesn't work because drivers don't like it if we activate an incomplete fbo.
On Mon, Apr 7, 2008 at 2:30 AM, Alexander Dorofeyev <alexd4(a)inbox.lv> wrote:
Oh, you are right. Must be fixed some other way then, I guess, maybe moving assignment to isInDraw lower or moving ActivateContext higher. From what I understood from Bioshock bug (http://bugs.winehq.org/show_bug.cgi?id=9973), moving ActivateContext in drawPrimitive produces bugs in other programs, right? Could you add to that bug a short description of what games etc needed to reproduce these problems?
I added a comment to the bug, but I'm afraid I don't really know the details about how to reproduce the problem as I never experienced it. I think Stefan was the one who mentioned the rendering problems in HL2 Lost Coast, so maybe he can give you more details? On another note, what are the thoughts about leaving these ActivateContext calls the way they are now? As things currently stand, a lot of these calls are only interested in making sure there is an active gl context by doing ActivateContext(device, device->lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD). This is essentially a NOP if there was a previous call to ActivateContext, so if you have something like: ActivateContext(device, target, usage); ... ENTER_GL(); ... ActivateContext(device, lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD); ... LEAVE_GL(); the second ActivateContext should be harmless, even if it's not ideal from a design standpoint. - Allan
There's a little problem with consistency about these things. In various related routines and paths, there were (and still are) both places that check isInDraw and places that don't check it. read_from_framebuffer and IWineD3DSurfaceImpl_BindTexture that can be called from LoadLocation are examples of each kind. Some kind of single scheme would make it easier to follow. Also even NOP ActivateContext(device, lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD) create some false positives when using debug traces to identify potential problem places where ActivateContext is used inside ENTER_GL/LEAVE_GL. On the other hand, it is pretty tricky. Also Stefan believes adding isInDraw checks everywhere isn't especially nice, but ultimately to prevent all situations of ActivateContext inside ENTER_GL/LEAVE_GL with isInDraw approach this would've to be added in more places. I'm not sure about this myself, perhaps I'll better not hurry with it and some better ideas may come. Allan Tong wrote:
I added a comment to the bug, but I'm afraid I don't really know the details about how to reproduce the problem as I never experienced it. I think Stefan was the one who mentioned the rendering problems in HL2 Lost Coast, so maybe he can give you more details?
On another note, what are the thoughts about leaving these ActivateContext calls the way they are now? As things currently stand, a lot of these calls are only interested in making sure there is an active gl context by doing ActivateContext(device, device->lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD). This is essentially a NOP if there was a previous call to ActivateContext, so if you have something like:
ActivateContext(device, target, usage); ... ENTER_GL(); ... ActivateContext(device, lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD); ... LEAVE_GL();
the second ActivateContext should be harmless, even if it's not ideal from a design standpoint.
- Allan
Am Dienstag, 8. April 2008 00:35:14 schrieb Alexander Dorofeyev:
On the other hand, it is pretty tricky. Also Stefan believes adding isInDraw checks everywhere isn't especially nice, but ultimately to prevent all situations of ActivateContext inside ENTER_GL/LEAVE_GL with isInDraw approach this would've to be added in more places. I'm not sure about this myself, perhaps I'll better not hurry with it and some better ideas may come. The original idea of this flag was to mark that the stateblock is finalized. It was never intended to give information about the opengl state(which this patches are doing), so if we're doing that we have to work out some ruleset about it.
participants (3)
-
Alexander Dorofeyev -
Allan Tong -
Stefan Dösinger