wined3d: Fix dead-loop condition with default setup.

[Stefan Dösinger]

Am Sonntag, 27. April 2008 21:21:22 schrieb Vitaliy Margolen:
> Vitaliy Margolen wrote:
> > As discussed in bug 11584 this Preload is wrong and should be removed.
> > ---
> >  dlls/wined3d/context.c |    7 -------
> >  1 files changed, 0 insertions(+), 7 deletions(-)
This patch almost certainly correct, but I think it shouldn't be applied 
without further investigation. There was a reason I added this PreLoad 
call(prior to multithreading support, and this issue occurs with 
multithreading), and I am not sure if we can remove it yet. Unfortunately my 
time is a bit limited currently.

Essentially, we have to call PreLoad(or well: GL calls to read the GL drawable 
into the surface's GL texture) because the old rendertarget surface looses 
control over the drawable and a new surface may change the GL drawable's 
contents.

We have to call ActivateContext in PreLoad because the readback code needs a 
GL context to work. The GL state should be correctly set up in a single 
threaded environment, but in case of multithreading there might be no 
context.

Both those issues are comparably easy to solve on their own. Issues now occur 
when the render target and thread are switched at the same time. In this 
case, we have to activate a context with the *old* drawable in the *new* 
thread, read back, and then activate the new drawable in the new thread.

The PreLoad call you're removing here is obviously flawed. It results in a 
readback attempt needlessly on a thread switch while offscreen rendering. We 
don't have to read back on a thread change, only on a render target change. 
Since lastActiveThread is set after the FindContext call(this is OK), it will 
infinitly recurse. This PreLoad call also shouldn't be needed, but there may 
be other bugs which require this call(It's aim is to create the texture).

The other PreLoad call is OK. I think it should be protected with a check if 
the render target is changed to prevent readback on a pure thread change. In 
this case, the above target-and-thread change will automagically solve itself

1) ActivateContext is called for a new render target and thread
2) FindContext sees the RT change. It calls PreLoad
3) PreLoad calls activatecontext on the new thread and old target
4) The recursed activateContext detects a thread only change. No PreLoad 
called this time. The old target is activated for readback in the new thread
5) LoadLocation performs the GL readback
6) PreLoad returns
7) ActivateContext continues to activate the new RT in the new thread

An issue is that this will be a rather implicit solution which is hard to see 
and tricky to verify. We still recurse, but the recursion limits itself and 
solves the problem.

A few test case suggestions:
*) Render onscreen, switch thread, switch thread back
*) Create an offscreen target, PreLoad it, render to it
*) Same as above, without the PreLoad
*) Create an offscreen target, render to it, switch thread(render there again)
*) Create an offscreen target, PreLoad, switch thread while switching RT
*) Same as above, without the PreLoad

The "render to it" could be just a Clear() call. However, the texture's 
contents should be checked, so that testing would be part of the visual test. 
Another trouble is that multithreading causes major pain with fglrx(fix seems 
to be on the way) and Mesa. So adding this to our testsuite at this point 
might upset many people who run make test by hard crashing their systems.