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.