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

[Paul Berry]

On 11 September 2012 16:24, Kenneth Graunke <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>
> Signed-off-by: Kenneth Graunke <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.

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120913/e832a853/attachment-0001.html>