2009/9/10 Stefan Dösinger stefan@codeweavers.com:
@@ -402,7 +433,7 @@ static void context_apply_fbo_entry(struct wined3d_context *context, struct fbo_ for (i = 0; i < GL_LIMITS(buffers); ++i) { IWineD3DSurface *render_target = device->render_targets[i];
context_attach_surface_fbo(context, GL_FRAMEBUFFER_EXT, i, render_target);
context_attach_surface_fbo(context, GL_FRAMEBUFFER_EXT, i, render_target, SRGB_ANY); } /* Apply depth targets */
I don't think SRGB_ANY is correct here. context_apply_fbo_entry() very much cares about which texture is attached to the FBO. If rendering to sRGB textures was supported properly this would depend on the sRGB write state, but since it isn't this should aways be SRGB_RGB.
@@ -2041,7 +2072,7 @@ struct wined3d_context *ActivateContext(IWineD3DDeviceImpl *This, IWineD3DSurfac FIXME("Activating for CTXUSAGE_BLIT for an offscreen target with ORM_FBO. This should be avoided.\n"); ENTER_GL(); context_bind_fbo(context, GL_FRAMEBUFFER_EXT, &context->dst_fbo);
context_attach_surface_fbo(context, GL_FRAMEBUFFER_EXT, 0, target);
context_attach_surface_fbo(context, GL_FRAMEBUFFER_EXT, 0, target, SRGB_ANY); context_attach_depth_stencil_fbo(context, GL_FRAMEBUFFER_EXT, NULL, FALSE); LEAVE_GL(); } else {
Similar to above.
@@ -6045,7 +6045,7 @@ static void color_fill_fbo(IWineD3DDevice *iface, IWineD3DSurface *surface, context = ActivateContext(This, NULL, CTXUSAGE_RESOURCELOAD); ENTER_GL(); context_bind_fbo(context, GL_FRAMEBUFFER_EXT, &context->dst_fbo);
context_attach_surface_fbo(context, GL_FRAMEBUFFER_EXT, 0, surface);
context_attach_surface_fbo(context, GL_FRAMEBUFFER_EXT, 0, surface, SRGB_ANY); context_attach_depth_stencil_fbo(context, GL_FRAMEBUFFER_EXT, NULL, FALSE);
Afaik color fill colors are specified in RGB space. Unless you have a test that shows otherwise, this should be SRGB_RGB.
@@ -6463,7 +6463,7 @@ void stretch_rect_fbo(IWineD3DDevice *iface, IWineD3DSurface *src_surface, WINED TRACE("Source surface %p is offscreen\n", src_surface); ENTER_GL(); context_bind_fbo(context, GL_READ_FRAMEBUFFER_EXT, &context->src_fbo);
context_attach_surface_fbo(context, GL_READ_FRAMEBUFFER_EXT, 0, src_surface);
context_attach_surface_fbo(context, GL_READ_FRAMEBUFFER_EXT, 0, src_surface, SRGB_ANY); glReadBuffer(GL_COLOR_ATTACHMENT0_EXT); checkGLcall("glReadBuffer()"); context_attach_depth_stencil_fbo(context, GL_READ_FRAMEBUFFER_EXT, NULL, FALSE);@@ -6503,7 +6503,7 @@ void stretch_rect_fbo(IWineD3DDevice *iface, IWineD3DSurface *src_surface, WINED
ENTER_GL(); context_bind_fbo(context, GL_DRAW_FRAMEBUFFER_EXT, &context->dst_fbo);
context_attach_surface_fbo(context, GL_DRAW_FRAMEBUFFER_EXT, 0, dst_surface);
context_attach_surface_fbo(context, GL_DRAW_FRAMEBUFFER_EXT, 0, dst_surface, SRGB_ANY); glDrawBuffer(GL_COLOR_ATTACHMENT0_EXT); checkGLcall("glDrawBuffer()"); context_attach_depth_stencil_fbo(context, GL_DRAW_FRAMEBUFFER_EXT, NULL, FALSE);
This doesn't work either. While it probably doesn't matter if the blit is done in RGB or sRGB space, it still needs to be the same color space for both surfaces (and one of those can be onscreen).
On a higher level, I'm not really happy with how sRGB textures currently fit in and how that's starting to spread to everything that has to deal with surfaces.
Am Friday 11 September 2009 00:05:02 schrieb Henri Verbeet:
I don't think SRGB_ANY is correct here. context_apply_fbo_entry() very much cares about which texture is attached to the FBO. If rendering to sRGB textures was supported properly this would depend on the sRGB write state, but since it isn't this should aways be SRGB_RGB.
Rendering to the RGB or sRGB copy of the surface isn't related to the srgb write state. It's only related to how the surface is read. An application can render to a texture with srgb writes off and read the texture with srgb read correction on. In this case we want to render to the srgb copy, otherwise we have to copy it later.
Whether you render to the RGB or sRGB copy doesn't affect the resulting data written into the texture, only performance(in the sense that a copy is needed or not needed).
This doesn't work either. While it probably doesn't matter if the blit is done in RGB or sRGB space, it still needs to be the same color space for both surfaces (and one of those can be onscreen).
All drivers I found so far can blit between sRGB and RGB textures, and it does what we want(doesn't change the data in the texture), so there's no problem if we blit from an RGB onscreen surface to an sRGB offscreen surface. If this didn't work the entire [3/4] patch would be moot.
On a higher level, I'm not really happy with how sRGB textures currently fit in and how that's starting to spread to everything that has to deal with surfaces.
I am not happy with this either, but I don't see this issue going away anytime soon. I'm always open to alternative suggestions though.
Two alternative ideas I had and threw away: -> Handle this in the texture, create two set of surfaces for each texture. Somewhat in the way the first crossover hack for this issue worked. That causes troubles if the app updates a surface, since we have to decide in the getSurfaceLevel call where all updates in the future will go. We'll also have to duplicate the location management in textures.
-> Handle this in d3d9, and create two wined3d textures/surfaces/volumes. Sounds nice at first because ddraw, d3d8 and as far as I can see d3d10 don't have this problem in the same way d3d9 does. But it would run into the same issues as the idea above, we'd have to duplicate the location management in d3d9, and maybe duplicate the stateblock management in d3d9(to keep track of D3DSAMP_SRGBTEXTURE, I am not sure if a GetSamplerState call is enough)
-> Some more generic system of having multiple GL textures per d3d texture/surface. I was thinking about that when we had issues with a texture being read on different samplers with different sampler states. However, I never had a good idea what this might look like, nor how this would be more elegant in the end.
-
Henri Verbeet -
Stefan Dösinger