On 11 September 2012 16:24, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Fixes colorspace issues in L4D2 when multisampling is enabled (the scene was far too dark, but the flashlight area was way too bright). NOTE: This is a candidate for the 9.0 branch. Cc: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> </blockquote><div> If I'm reading the spec correctly, it's awfully contradictory. From GL 4.3 compatibility profile, section 18.3.1 Blitting Pixel Rectangles: (1) When values are taken from the read buffer, if the value of FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING for the framebuffer attachment corresponding to the read buffer is SRGB (see section 9.2.3), the red, green, and blue components are converted from the non-linear sRGB color space according to equation 8.14. (2) When values are taken from the read buffer, no linearization is performed even if the format of the buffer is SRGB. (3) When values are written to the draw buffers, blit operations bypass most of the fragment pipeline. The only fragment operations which affect a blit are the pixel ownership test, the scissor test, and sRGB conversion (see section 17.3.9). Color, depth, and stencil masks (see section 17.4.2) are ignored. And then a few paragraphs down: (4) If SAMPLE_BUFFERS for either the read framebuffer or draw framebuffer is greater than zero, no copy is performed and an INVALID_OPERATION error is generated if the dimensions of the source and destination rectangles provided to BlitFramebuffer are not identical, or if the formats of the read and draw framebuffers are not identical. Also, from section 17.3.9 sRGB Conversion: (5) If FRAMEBUFFER_SRGB is enabled and the value of FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING for the framebuffer attachment corresponding to the destination buffer is SRGB1 (see section 9.2.3), the R, G, and B values after blending are converted into the non-linear sRGB color space by computing ... [formula follows] ... If FRAMEBUFFER_SRGB is disabled or the value of FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING is not SRGB, then ... [no conversion is applied]. Paragraphs (1) and (2) seem irreconcilable: the first says that linearization should happen when reading from SRGB buffers, the second says that it shouldn't. Or am I misreading something? If we read paragraphs (1), (3), (4), and (5) together, the spec seems to say that: linear -> linear should be a direct copy. linear -> sRGB should be prohibited if either buffer is multisampled; if it's allowed, it should be a direct copy if FRAMEBUFFER_SRGB is disabled, and it should convert into sRGB space if FRAMEBUFFER_SRGB is enabled. sRGB -> linear should be prohibited if either buffer is multisampled; if it's allowed, it should convert out of sRGB space. sRGB -> sRGB should be a direct copy if FRAMEBUFFER_SRGB is enabled, and it should convert out of sRGB space if FRAMEBUFFER_SRGB is disabled. This seems kinda crazy at first blush, since FRAMEBUFFER_SRGB is disabled by default, which means that the default behaviour for sRGB -> sRGB won't be a direct copy, and that's really counterintuitive. Then again, perhaps programs that blit to sRGB framebuffers enable FRAMEBUFFER_SRGB because of this. If we read paragraphs (2), (3), (4), and (5) together, the spec seems to say that: linear -> linear should be a direct copy. linear -> sRGB should be prohibited if either buffer is multisampled; if it's allowed, it should be a direct copy if FRAMEBUFFER_SRGB is disabled, and it should convert into sRGB space if FRAMEBUFFER_SRGB is enabled. sRGB -> linear should be prohibited if either buffer is multisampled; if it's allowed, it should be a direct copy. sRGB -> sRGB should be a direct copy if FRAMEBUFFER_SRGB is disabled, and it should convert into sRGB space if FRAMEBUFFER_SRGB is enabled. This seems a little less crazy in light of the fact that FRAMEBUFFER_SRGB is disabled by default, since it makes all blits a direct copy under default conditions. Note: in both of the above interpretations, I'm assuming that sRGB and linear formats are not "identical" for the purpose of interpreting paragraph (4). It's conceivable that other implementations have adopted a more liberal view that treats sRGB and 8-bit linear as "identical" formats, but that really seems like a stretch. What's your interpretation of the spec? Your patch series looks like it will do a direct copy in all cases, regardless of the setting of FRAMEBUFFER_SRGB, and it will allow linear -> sRGB and sRGB -> linear regardless of whether the buffers are multisampled. I'm really torn about whether to give these patches my Reviewed-by. On the one hand, they get L4D2 to work, so they are probably a step in the right direction. On the other hand, I can't reconcile them with what I see in the spec. In an ideal world, before we went forward with this patch, I would really like to see a piglit test that runs through all the possibilities and reports whether each blit (a) fails, (b) does a direct copy, (c) converts sRGB to linear, or (d) converts linear to sRGB. Then we could try the piglit test on other implementations to make sure we're barking up the right tree. Do you have time to do this sort of testing before the 9.0 release? Could I help? (Note that for a thorough test, we should probably include blits to and from the default framebuffer. I've had the suspicion from previous experiments that nVidia bends some of the blitting rules when the default framebuffer is involved, to account for the fact that its default framebuffers are sRGB). </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- src/mesa/drivers/dri/i965/brw_blorp.cpp | 5 +++-- src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 7 +++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp b/src/mesa/drivers/dri/i965/brw_blorp.cpp index 9017e4d..e4451f3 100644 --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp @@ -97,8 +97,9 @@ brw_blorp_surface_info::set(struct brw_context *brw, * target, even if this is the source image. So we can convert to a * surface format using brw->render_target_format. */ - assert(brw->format_supported_as_render_target[mt->format]); - this->brw_surfaceformat = brw->render_target_format[mt->format]; + gl_format linear_format = _mesa_get_srgb_format_linear(mt->format); + assert(brw->format_supported_as_render_target[linear_format]); + this->brw_surfaceformat = brw->render_target_format[linear_format]; break; } } diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp index 1e49ac5..c6c24b1 100644 --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp @@ -175,8 +175,11 @@ formats_match(GLbitfield buffer_bit, struct gl_renderbuffer *src_rb, * example MESA_FORMAT_X8_Z24 and MESA_FORMAT_S8_Z24), and we can blit * between those formats. */ - return find_miptree(buffer_bit, src_rb)->format == - find_miptree(buffer_bit, dst_rb)->format; + gl_format src_format = find_miptree(buffer_bit, src_rb)->format; + gl_format dst_format = find_miptree(buffer_bit, dst_rb)->format; + + return _mesa_get_srgb_format_linear(src_format) == + _mesa_get_srgb_format_linear(dst_format); } -- 1.7.11.4 </blockquote></div>