[1/4 try 3] wined3d: Activate GL context before making any GL calls in stretch_rect_fbo

[H. Verbeet]

Overall, the patches look pretty reasonable.

On 21/12/2007, Stefan Dösinger <stefandoesinger at 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.