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(-)
BTW this is still not enough to fix all the problems. There are few more preloads that make Psychonauts crash. So I think that whole idea of using preload in the context switch is not right. Or activating context in texture pre-loading is wrong.
Can some one familiar with this code take a closer look? This is basic functionality that should be working in 1.0
Vitaliy.
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(-)
BTW this is still not enough to fix all the problems. There are few more preloads that make Psychonauts crash. So I think that whole idea of using preload in the context switch is not right. Or activating context in texture pre-loading is wrong.
Can some one familiar with this code take a closer look? This is basic functionality that should be working in 1.0
Vitaliy.
The whole surface / texture code is quite tricky because of various reasons. First of I have the feeling various parts are still NIH and old (and perhaps not doing what it was originally intended for0. Another big source of troubles is that a bunch of different GL extensions are used (FBO, PBO, texture rectangle ..) in order to achieve certain effects or to improve performance. This causes various games to behave differently on different cards and because drivers aren't bug free, you can have different bugs while the same GL extensions are offered. All in all a hell.
I haven't dealt with much of the code recently but I tried to move common functionality in small functions to make the code more readable and easier to maintain.
Just removing preloads and so isn't an option I think. I think the best thing would be to carefully review the code and to see what a function should be doing and make the code easier and better. Right now what is a fix for game 1 is a regression for game 2. I have seen this a couple of times right now where I fixed some large bugs but which exposed other issues.
The main issue right now is that the code freeze is near and this really isn't anything trivial and you can't write test cases for it. Perhaps native wined3d test apps could be employed but as I said there are dozens of different codepaths which can be entered due to differences in GL features.
Regards, Roderick
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.
-
Roderick Colenbrander -
Stefan Dösinger -
Vitaliy Margolen