On 17 September 2012 10:25, Ian Romanick <i...@freedesktop.org> wrote:
> On 09/14/2012 02:56 AM, Paul Berry wrote: > >> On 11 September 2012 16:24, Kenneth Graunke <kenn...@whitecape.org >> <mailto:kenn...@whitecape.org>**> wrote: >> >> 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 <stereotype...@gmail.com >> <mailto:stereotype441@gmail.**com <stereotype...@gmail.com>>> >> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org >> <mailto:kenn...@whitecape.org>**> >> >> >> >> 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. >> > > A bunch of these language changes are new, and there seem to be some > issues. Basically, sRGB and blits have been horribly broken since the > beginning, and fixing it to be sensible is breaking apps. Wine, for > example, is pretty pissed about this. Note that *none* of the > closed-source drivers implement the behavior from the spec. *None*. Do you know what behaviour is typically implemented or should we go out and do some tests? > > > 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). >> >> --- >> 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 >> > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
