[Mesa-dev] [PATCH 2/2] i965/blorp: Add support for blits between SRGB and linear formats.
Jordan Justen
jljusten at gmail.com
Mon Sep 17 09:40:38 PDT 2012
I have nothing to add regarding Paul's concern, but these 2 patches are
Tested-by: Jordan Justen <jordan.l.justen at intel.com>
for gen6/gen7.
-Jordan
On Thu, Sep 13, 2012 at 5:56 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> 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
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list