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

[Allan Tong]

On Dec 21, 2007 9:35 AM, H. Verbeet <hverbeet at gmail.com> wrote:
> 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.

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