Re: [1/4 try 3] wined3d: Activate GL context before making any GL calls in stretch_rect_fbo
Hi,
I think your patches are on the right track, but there are a few issues to look at. I'll also need Henri's comments, as he wrote the fbo code.
Did you try your patches with the Deferred Shading sample from codesampler? http://www.codesampler.com/usersrc/usersrc_7.htm . It uses multiple render targets, so it is a quite interesting test.
drawprimitive() in drawprim.c calls apply_fbo_state before calling ActivateContext as well. This causes a lot of problems with multithreading, so this should be fixed too.
The first patch is OK. Calling ActivateContext before activating the fbo is required.
In patch 2, I was a bit confused by moving the "This->activeContext" assignment inside Init3D, I first thought you would remove it. But otherwise it looks good to me as well.
Patch 3 is ok too, keeping track of the currently bound fbo in the context is good. Unrelated to this patch we should look if the glBindFramebufferEXT(..., 0) calls could be replaced with calling ActivateContext, and if that makes sense(it's probably not a good idea everywhere).
In patch 4, you have to be aware that in CTXUSAGE_BLIT the fbo's depth and color1+ attachments might be set to bad textures(e.g. different size). So you'll have to set them to NULL or make sure that they are set properly.
I once discussed with Henri where we should have all the functions. I think we could just move apply_fbo_state to context.c, make it static to make sure the code that requires a drawable calls ActivateContext. I am not entirely sure if that makes sense everywhere, e.g. the depth_blit code. Henri had some arguments in that regard, but I have to check the irc logs and digg out the discussion.
Other good testing applications are Half Life 2(demo is ok), it uses color buffers and depth buffers with different sizes. Battlefield 2(not 2142, no idea if there is a demo) depends on the depth blitting. Bioshock(demo is ok) is a case of an application that crashes due to multithreading issues because apply_fbo_state is applied before calling ActivateContext.
Thank you for all your excellent work, Stefan
Overall, the patches look pretty reasonable.
On 21/12/2007, Stefan Dösinger stefandoesinger@gmx.at wrote:
Did you try your patches with the Deferred Shading sample from codesampler? http://www.codesampler.com/usersrc/usersrc_7.htm . It uses multiple render targets, so it is a quite interesting test.
Keep in mind that that one needs a couple of hacks to the format table to work properly though.
drawprimitive() in drawprim.c calls apply_fbo_state before calling ActivateContext as well. This causes a lot of problems with multithreading, so this should be fixed too.
Unless, I misunderstand the issue, that one should be taken care of in the 4th patch already.
Patch 3 is ok too, keeping track of the currently bound fbo in the context is good. Unrelated to this patch we should look if the glBindFramebufferEXT(..., 0) calls could be replaced with calling ActivateContext, and if that makes sense(it's probably not a good idea everywhere).
Personally, I don't like keeping track of flags like this all over the place:
GL_EXTCALL(glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0));
This->activeContext->last_fbo = 0;
I'd rather ban naked glBindFramebufferEXT() calls and either allow bind_fbo() to accept NULL for the fbo parameter, or create an unbind_fbo() function. Another option could be to handle framebuffer selection completely in ActivateContext(). Arguably that's the proper way to do it, but it would be a bit more involved, and would require extending ActivateContext with a "source_surface" parameter, to handle stretch_rect_fbo(), and possibly a few more CTXUSAGE types.
In patch 4, you have to be aware that in CTXUSAGE_BLIT the fbo's depth and color1+ attachments might be set to bad textures(e.g. different size). So you'll have to set them to NULL or make sure that they are set properly.
Currently dst_fbo will only ever have an attachment at GL_COLOR_ATTACHMENT0_EXT, if any. That could potentially change if we would start to handle colorfill on depth stencils or stretch_rect_fbo() between them, but I don't expect that to happen very soon.
What I do wonder about is why CTXUSAGE_BLIT even binds dst_fbo though. It's certainly not used in any of the places that currently use dst_fbo, and I'm not sure it makes sense to bind dst_fbo in any of the places that does call ActivateContext that way.
I once discussed with Henri where we should have all the functions. I think we could just move apply_fbo_state to context.c, make it static to make sure the code that requires a drawable calls ActivateContext. I am not entirely sure if that makes sense everywhere, e.g. the depth_blit code. Henri had some arguments in that regard, but I have to check the irc logs and digg out the discussion.
IIRC the issue was with moving the depth blit itself to ActivateContext(). I think just selecting the proper framebuffer etc. should be fine.
Other good testing applications are Half Life 2(demo is ok), it uses color buffers and depth buffers with different sizes. Battlefield 2(not 2142, no idea if there is a demo) depends on the depth blitting. Bioshock(demo is ok) is a case of an application that crashes due to multithreading issues because apply_fbo_state is applied before calling ActivateContext.
BF2142 does have a demo, but afaik it allows online play only, which makes testing a bit awkward. It doesn't need depth blitting like BF2, but does have HDR effects, IIRC. Both BF2 and BF2142 require support for animated cursors.
On Dec 21, 2007 9:35 AM, H. Verbeet hverbeet@gmail.com wrote:
On 21/12/2007, Stefan Dösinger stefandoesinger@gmx.at wrote:
Did you try your patches with the Deferred Shading sample from codesampler? http://www.codesampler.com/usersrc/usersrc_7.htm . It uses multiple render targets, so it is a quite interesting test.
Keep in mind that that one needs a couple of hacks to the format table to work properly though.
I didn't try it. Do you know what hacks are necessary?
In patch 2, I was a bit confused by moving the "This->activeContext" assignment inside Init3D, I first thought you would remove it. But otherwise it looks good to me as well.
I was running into a problem where last_draw_buffer was not set to anything right after device initialization, but I'm not sure if it's still a problem with the current patches. It may be an issue if for example after the device is initialized, ActivateContext is only called on lastActiveRenderTarget (front or back buffer). If some code then bound a FBO outside of ActivateContext, the next call to ActivateContext (again with lastActiveRenderTarget) would try resetting the framebuffer binding to 0 and set the draw buffer to context->last_draw_buffer since lastActiveRenderTarget still hasn't changed, but context->last_draw_buffer is still 0 since we never updated it after initializing the device.
Patch 3 is ok too, keeping track of the currently bound fbo in the context is good. Unrelated to this patch we should look if the glBindFramebufferEXT(..., 0) calls could be replaced with calling ActivateContext, and if that makes sense(it's probably not a good idea everywhere).
Personally, I don't like keeping track of flags like this all over the place:
GL_EXTCALL(glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0));
This->activeContext->last_fbo = 0;I'd rather ban naked glBindFramebufferEXT() calls and either allow bind_fbo() to accept NULL for the fbo parameter, or create an unbind_fbo() function. Another option could be to handle framebuffer selection completely in ActivateContext(). Arguably that's the proper way to do it, but it would be a bit more involved, and would require extending ActivateContext with a "source_surface" parameter, to handle stretch_rect_fbo(), and possibly a few more CTXUSAGE types.
I guess I could see it getting out of hand, especially since this patch only tracks fbos, but not the fbo attachments or even any glDrawBuffer calls made outside of ActivateContext, all of which are potentially problematic for ActivateContext. I like the idea of banning the use of glBindFramebuffer/glDrawBuffer/glFramebufferTexture/glFramebufferRenderbuffer outside of ActivateContext, but that may not be possible. Maybe just try to merge as many of these calls into ActivateContext as can be easily done and have a strategy in place for the rest, e.g. code that changes fbo or draw buffer state has to either update wined3d state to match the new GL state or it needs to make sure that it restores the GL state so that it agrees with what wined3d thinks the GL state is.
What I do wonder about is why CTXUSAGE_BLIT even binds dst_fbo though. It's certainly not used in any of the places that currently use dst_fbo, and I'm not sure it makes sense to bind dst_fbo in any of the places that does call ActivateContext that way.
I was looking at some places that currently don't bind to dst_fbo, e.g. flush_to_framebuffer_drawpixels. It looks wrong to me for offscreen fbo surfaces, but admittedly I'm not too familiar with when that's used.
Other good testing applications are Half Life 2(demo is ok), it uses color buffers and depth buffers with different sizes. Battlefield 2(not 2142, no idea if there is a demo) depends on the depth blitting. Bioshock(demo is ok) is a case of an application that crashes due to multithreading issues because apply_fbo_state is applied before calling ActivateContext.
BF2142 does have a demo, but afaik it allows online play only, which makes testing a bit awkward. It doesn't need depth blitting like BF2, but does have HDR effects, IIRC. Both BF2 and BF2142 require support for animated cursors.
I tried Half Life 2 and Bioshock. I'll take a look at BF2.
Thanks for all the comments.
- Allan
-
Allan Tong -
H. Verbeet -
Stefan Dösinger