2009/12/10 Stefan Dösinger stefan@codeweavers.com:
@@ -6304,7 +6304,7 @@ HRESULT create_primary_opengl_context(IWineD3DDevice *iface, IWineD3DSwapChain *
...
- swapchain->context[0]->render_offscreen = swapchain->render_to_fbo;
- swapchain->context[0]->render_offscreen = surface_is_offscreen(target);
This will introduce a warning, surface_is_offscreen() takes an IWineD3DSurface pointer.
swapchain->context[0]->render_offscreen = swapchain->render_to_fbo;
...
if(swapchain->context[0]){swapchain->context[0]->render_offscreen = swapchain->render_to_fbo;}
That's pretty ugly, couple of points: - It doesn't make sense for "render_to_fbo" to be TRUE for a swapchain without back buffer. - Setting "render_offscreen" is really the responsibility of the context_create() call, similar to create_primary_opengl_context(). - The line is actually wrong, the relevant context has been setup to render to the front buffer, so "render_offscreen" should always be FALSE, even if there is a back buffer.
Am 10.12.2009 um 23:15 schrieb Henri Verbeet:
2009/12/10 Stefan Dösinger stefan@codeweavers.com:
@@ -6304,7 +6304,7 @@ HRESULT create_primary_opengl_context(IWineD3DDevice *iface, IWineD3DSwapChain *
...
- swapchain->context[0]->render_offscreen = swapchain->render_to_fbo;
- swapchain->context[0]->render_offscreen = surface_is_offscreen(target);
This will introduce a warning, surface_is_offscreen() takes an IWineD3DSurface pointer.
Woops, I guess I should not use my own gcc all the time which chokes on the OSX headers and writes warning spam :-/
- Setting "render_offscreen" is really the responsibility of the
context_create() call, similar to create_primary_opengl_context().
context_create() doesn't take a swapchain as parameter, only a window. This is correct, because GL contexts are linked to the drawable, not the swapchain. Thus context_create doesn't have the needed information to set the offscreen flag. (I could just pass in a TRUE or FALSE parameter, but there's something else)
The reason why this line is needed however is because the context has a last render target set, and if the app never changes the render target the sorta-cached render-offscreen value never gets set properly. Maybe we should make sure that the full FindContext() runs at least once for a newly created context, then we can drop this line and the one in create_primary_opengl_context(). Should keeping the last_rt set to NULL after context creation work correctly?
As a sidenode, maybe create_primary_opengl_context is named badly. It's not part of the context management, its a device helper function that happens to use the context management to set up a GL context, among other things. Maybe device_restart_opengl or something similar is better.
- The line is actually wrong, the relevant context has been setup to
render to the front buffer, so "render_offscreen" should always be FALSE, even if there is a back buffer.
Well, this code is in the backbuffer path. Render_offscreen is false if we don't set it due to the alloc HEAP_ZERO_MEMORY. The point of the line is to set it to true if we render to an FBO. We never set a swapchain to render to the front buffer at creation time if there is a back buffer.
- It doesn't make sense for "render_to_fbo" to be TRUE for a
swapchain without back buffer.
Correct, but this needs another patch to fix.
First of all, render_to_fbo shouldn't matter when we're dealing with the front buffer, even when a back buffer is there. surface_is_offscreen takes care of that, but we have to use it. This is important when we're forcing render_to_fbo on for driver bug workarounds, or if we have a double buffered environment with render_to_fbo on for a real(non-driver) reason and access the front buffer due to some ddraw/d3d9ex tricks or wined3d internal stuff.
In the ddraw windowed mode rendering situation rendering the backbuffer to a FBO is actually the correct thing to do, because from the application's point of view the backbuffer is an offscreen plain surface. We just construct a double-buffered GL setup to be able to use windowed ddraw rendering without FBOs on older drivers. Currently this triggers a few other bugs(e.g. in GetDC() + Client_storage) that I want to fix first.
So the next step after fixing the GL errors and the GetDC bug is to ignore the back buffer size in CreateSwapchain and Reset if there is no backbuffer. Then render_to_fbo will be false for single buffered setups.
The academic question afterwards is if we render windowed d3d7 setups to an FBO if available or not. If we have FBOs, the difference is small. I haven't seen an app that depends on this yet, and unless we are on MacOS 10.5 or earlier which cannot write to the front buffer I don't expect driver issues either. This whole thing is done do please drivers without FBOs.
So once the other bugs are fixed I'll add a check to CreateSwapchain and reset to ignore the backbuffer size if backbuffercount == 0
2009/12/11 Stefan Dösinger stefandoesinger@gmx.at:
- Setting "render_offscreen" is really the responsibility of the context_create() call, similar to create_primary_opengl_context().
context_create() doesn't take a swapchain as parameter, only a window. This is correct, because GL contexts are linked to the drawable, not the swapchain. Thus context_create doesn't have the needed information to set the offscreen flag. (I could just pass in a TRUE or FALSE parameter, but there's something else)
It has a "target" parameter that can be either an onscreen or offscreen surface. This parameter is also used to set the initial "current_rt" field. "similar to create_primary_opengl_context()" was perhaps badly worded, what I meant is that context_create() should be setting "render_offscreen", not its callers.
As a sidenode, maybe create_primary_opengl_context is named badly. It's not part of the context management, its a device helper function that happens to use the context management to set up a GL context, among other things. Maybe device_restart_opengl or something similar is better.
Yeah.
- The line is actually wrong, the relevant context has been setup to render to the front buffer, so "render_offscreen" should always be FALSE, even if there is a back buffer.
Well, this code is in the backbuffer path. Render_offscreen is false if we don't set it due to the alloc HEAP_ZERO_MEMORY. The point of the line is to set it to true if we render to an FBO. We never set a swapchain to render to the front buffer at creation time if there is a back buffer.
"context_create(device, (IWineD3DSurfaceImpl *)swapchain->frontBuffer, window, FALSE /* pbuffer */, present_parameters);"
The draw buffer handling in swapchain_init() is also broken in the sense that it always sets GL_BACK if there's a back buffer, regardless of what the context thinks it should be, but that's a separate issue, I'm working on some patches to fix draw buffer handling in the general case.
Am 11.12.2009 um 11:33 schrieb Henri Verbeet:
"context_create(device, (IWineD3DSurfaceImpl *)swapchain->frontBuffer, window, FALSE /* pbuffer */, present_parameters);"
What is missing is access to the swapchain->render_to_fbo setting. context_create could use the same logic as the swapchain_create code(look at window and backbuffer size etc). However, I prefer not to initialize render_offscreen during context creation, and instead keep context->current_rt set to NULL until the context is actually used the first time. This way context->render_offscreen will be set the first time the context is used. The swapchain is fully initialized at this point, and we don't run into the same problem the next time we add a field similar to render_offscreen to the context.
2009/12/11 Stefan Dösinger stefandoesinger@gmx.at:
Am 11.12.2009 um 11:33 schrieb Henri Verbeet:
"context_create(device, (IWineD3DSurfaceImpl *)swapchain->frontBuffer, window, FALSE /* pbuffer */, present_parameters);"
What is missing is access to the swapchain->render_to_fbo setting. context_create could use the same logic as the swapchain_create code(look at window and backbuffer size etc).
It's really not that hard, you just call surface_is_offscreen() on the target. Arguably swapchain_init() shouldn't be screwing around with context creation in the first place, but that's really a separate issue, unless you're volunteering to clean that up first.
Am 11.12.2009 um 15:47 schrieb Henri Verbeet:
It's really not that hard, you just call surface_is_offscreen() on the target. Arguably swapchain_init() shouldn't be screwing around with context creation in the first place, but that's really a separate issue, unless you're volunteering to clean that up first.
By the time we create the context the swapchain and surfaces aren't fully constructed. However, we luckily call SetContainer on the initial surface before we call create_context, so it should work for now.
(Incidentally, we create the context with the front buffer as target, so context_acquire with the back buffer as target should set context->render_offscreen correctly anyway. For some reason though it doesn't (Or did not, when I wrote the render-to-fbo code. Or maybe that was after a Reset() call)
2009/12/11 Stefan Dösinger stefandoesinger@gmx.at:
Am 11.12.2009 um 15:47 schrieb Henri Verbeet:
It's really not that hard, you just call surface_is_offscreen() on the target. Arguably swapchain_init() shouldn't be screwing around with context creation in the first place, but that's really a separate issue, unless you're volunteering to clean that up first.
By the time we create the context the swapchain and surfaces aren't fully constructed. However, we luckily call SetContainer on the initial surface before we call create_context, so it should work for now.
That's a big part of the reason why swapchain_init() shouldn't do context creation. Doesn't change the fact that it's the responsibility of context_create() to properly initialize the context's fields.
-
Henri Verbeet -
Stefan Dösinger