Re: [5/5] WineD3D: Some render target->texture blit context handling
On 08/06/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
+ ActivateContext(This, src_surface, CTXUSAGE_RESOURCELOAD); + if (GL_SUPPORT(ARB_MULTITEXTURE)) { + GL_EXTCALL(glActiveTextureARB(GL_TEXTURE0_ARB)); + checkGLcall("glActiveTextureARB"); + } + IWineD3DDeviceImpl_MarkStateDirty(This, STATE_SAMPLER(0)); + IWineD3DSurface_PreLoad(dst_surface);
Why do you need this? The surface should be preloaded in attach_surface_fbo as soon as it's attached. Also, although stretch_rect_fbo() is currently only used for onscreen -> texture blits, it should not be limited to that, so I think the ActivateContext should be further down inside both the "if (src_swapchain)" and "if (dst_swapchain)" blocks.
Am Freitag, 8. Juni 2007 17:12 schrieb H. Verbeet:
On 08/06/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
+ ActivateContext(This, src_surface, CTXUSAGE_RESOURCELOAD); + if (GL_SUPPORT(ARB_MULTITEXTURE)) { + GL_EXTCALL(glActiveTextureARB(GL_TEXTURE0_ARB)); + checkGLcall("glActiveTextureARB"); + } + IWineD3DDeviceImpl_MarkStateDirty(This, STATE_SAMPLER(0)); + IWineD3DSurface_PreLoad(dst_surface);
Why do you need this? The surface should be preloaded in attach_surface_fbo as soon as it's attached. Also, although stretch_rect_fbo() is currently only used for onscreen -> texture blits, it should not be limited to that, so I think the ActivateContext should be further down inside both the "if (src_swapchain)" and "if (dst_swapchain)" blocks. I added the preload because the old code also preloaded in case of fbo blits. I didn't specifically look if the fbo code does that, though. If it does we can remove it, but I'd prefer to do it in a seperate patch to keep the changes seperated.
wrt the activateContext, its not only needed for the proper reading / writing to the framebuffer, but also to get a proper context for multithreading. This was the motivation for this patch.
On 08/06/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
I added the preload because the old code also preloaded in case of fbo blits. I didn't specifically look if the fbo code does that, though. If it does we can remove it, but I'd prefer to do it in a seperate patch to keep the changes seperated.
Fair enough, although it's redundant.
wrt the activateContext, its not only needed for the proper reading / writing to the framebuffer, but also to get a proper context for multithreading. This was the motivation for this patch.
Well yes, but inside stretch_rect_fbo() src_surface isn't guaranteed to be a render target (either onscreen or offscreen), so I'm not sure that calling ActivateContext on src_surface like that will always do what we want. In fact, neither src_surface nor dst_surface has to be a render target, so wouldn't it make more sense to do the context activation in BltOverride instead?
Well yes, but inside stretch_rect_fbo() src_surface isn't guaranteed to be a render target (either onscreen or offscreen), so I'm not sure that calling ActivateContext on src_surface like that will always do what we want. In fact, neither src_surface nor dst_surface has to be a render target, so wouldn't it make more sense to do the context activation in BltOverride instead? Activating the context in BltOverride is redundant too, because both the stretch and direct blit need the context activated for blitting, while stretch_rect need a RESOURCELOAD only. So either we're activating twice for non-fbo blits, or have an overkill for fbo blits. I'll check how stretch_rect_fbo is used in other places and put an ActivateContext(This->lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD) there if needed. This will make multithreading happy without requireing a special surface.
On 09/06/07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Well yes, but inside stretch_rect_fbo() src_surface isn't guaranteed to be a render target (either onscreen or offscreen), so I'm not sure that calling ActivateContext on src_surface like that will always do what we want. In fact, neither src_surface nor dst_surface has to be a render target, so wouldn't it make more sense to do the context activation in BltOverride instead? Activating the context in BltOverride is redundant too, because both the stretch and direct blit need the context activated for blitting, while stretch_rect need a RESOURCELOAD only. So either we're activating twice for non-fbo blits, or have an overkill for fbo blits. Well, it can go inside the if block that checks for EXT_framebuffer_blit.
I'll check how stretch_rect_fbo is used in other places and put an ActivateContext(This->lastActiveRenderTarget, CTXUSAGE_RESOURCELOAD) there if needed. This will make multithreading happy without requireing a special surface.
Afaik, it's currently only used in BltOverride for onscreen->texture copy, but it should imo be used for the other BltOverride cases as well.
participants (2)
-
H. Verbeet -
Stefan Dösinger