I have nothing to add regarding Paul's concern, but these 2 patches are Tested-by: Jordan Justen <jordan.l.jus...@intel.com>
for gen6/gen7. -Jordan On Thu, Sep 13, 2012 at 5:56 PM, Paul Berry <stereotype...@gmail.com> wrote: > On 11 September 2012 16:24, Kenneth Graunke <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> >> Signed-off-by: Kenneth Graunke <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. > > 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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
