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.