[Mesa-dev] [PATCH 2/2] i965/blorp: Add support for blits between SRGB and linear formats.

[Ian Romanick]

On 09/14/2012 02:56 AM, Paul Berry wrote:
> On 11 September 2012 16:24, Kenneth Graunke <kenneth at whitecape.org
> <mailto:kenneth at 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 <stereotype441 at gmail.com
>     <mailto:stereotype441 at gmail.com>>
>     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at 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*.

> 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