Overall, the patches look pretty reasonable. On 21/12/2007, Stefan Dösinger <[EMAIL PROTECTED]> 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.
