[Mesa-dev] [PATCH 2/2] i965/blorp: Add support for blits between SRGB and linear formats.
Paul Berry
stereotype441 at gmail.com
Mon Sep 17 10:53:41 PDT 2012
On 17 September 2012 10:25, Ian Romanick <idr at freedesktop.org> wrote:
> 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 <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*.
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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120917/cea0d643/attachment-0001.html>
More information about the mesa-dev
mailing list