[Mesa-dev] [PATCH 1/3] meta: Implement sensible behavior for BlitFramebuffer with sRGB.

Wed Aug 1 03:16:31 PDT 2012

On 01.08.2012 11:59, Christoph Bumiller wrote:
> On 01.08.2012 07:52, Kenneth Graunke wrote:
>> On 07/31/2012 06:04 PM, Eric Anholt wrote:
>>> Prior to GL 4.2 spec, there was no guidance on how to implement
>>> BlitFramebuffer for sRGB.  Mesa chose to implement skipping decode and encode,
>>> providing a reasonable behavior for sRGB -> sRGB or RGB -> RGB, but providing
>>> something absurd for mixing and matching the two.
>>>
>>> In GL 4.2, some text appeared in the spec saying to skip decode (search for
>>> "no linearization").  The only non-absurd interpretation of that would be to
>>> have no encode (same as Mesa's current implementation), otherwise sRGB -> sRGB
>>> blits wouldn't work.
>>>
>>> However, it seems clear that you should be able to blit sRGB to RGB and RGB to
>>> sRGB and get appropriate conversion.  The ARB has been discussing the issue,
>>> and appears to agree.  So, instead implement the same behavior as gallium, and
>>> always do the decode if the texture is sRGB, and do the encode if the
>>> application asked for it.
>>>
> 
> I was just about to revert that patch (gallium: specify resource_resolve
> destination via a pipe_surface), because it does not match the behaviour
> of the NVIDIA binary driver, and I discovered that I accidentally had
> sRGB decode disabled (by previously unidentified not-set bit in sampler
> state I didn't see the blob set for resolve), so disabling sRGB encode
> (which is what the app has set) counter-acted that and made rendering
> look correct.
> If fix the decode setting AND honor the write setting, I get wrong
> results again.

However, reading that part of the 4.2 spec (let me quote here):

"When values are taken from the read buffer, no linearization is
performed even if the format of the buffer is SRGB.
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 4.1.8)."

It looks like I accidentally did the *right* thing, too - never do
linearization == no sRGB decode on read. This seems a little odd though,
blits from sRGB to sRGB with FRAMEBUFFER_SRGB enabled would change the
values, which takes some getting used to ...

This would also requires another change to the gallium interface, making
the source never be an sRGB format, or at least stating that it is
supposed to be treated as linear.

> 
> That is, an sRGB -> sRGB blit should never change the values, regardless
> of the setting of GL_FRAMEBUFFER_SRGB.
> 
> EXT_framebuffer_sRGB suggests that only drawing (i.e. where you have
> blending) should be affected:
> 
> Should sRGB framebuffer support affect the pixel path?
> 
>         RESOLVED:  No.
> 
>         sRGB conversion only applies to color reads for blending and
>         color writes.  Color reads for glReadPixels, glCopyPixels,
>         or glAccum have no sRGB conversion applied.
> 
>>> Breaks piglit fbo-srgb-blit, which was expecting our previous no-conversion
>>> behavior.
>>> ---
>>>  src/mesa/drivers/common/meta.c |   14 +++++---------
>>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
>>> index 6846bbc..e35f0c8 100644
>>> --- a/src/mesa/drivers/common/meta.c
>>> +++ b/src/mesa/drivers/common/meta.c
>>> @@ -1357,7 +1357,6 @@ blitframebuffer_texture(struct gl_context *ctx,
>>>           const GLenum wrapSSave = texObj->Sampler.WrapS;
>>>           const GLenum wrapTSave = texObj->Sampler.WrapT;
>>>           const GLenum srgbSave = texObj->Sampler.sRGBDecode;
>>> -         const GLenum fbo_srgb_save = ctx->Color.sRGBEnabled;
>>>           const GLenum target = texObj->Target;
>>>  
>>>           if (drawAtt->Texture == readAtt->Texture) {
>>> @@ -1390,14 +1389,14 @@ blitframebuffer_texture(struct gl_context *ctx,
>>>           _mesa_TexParameteri(target, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
>>>           _mesa_TexParameteri(target, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
>>>  
>>> -	 /* Always do our blits with no sRGB decode or encode.*/
>>> +	 /* Always do sRGB decode on the read, and do sRGB encode on the write
>>> +          * if we've been asked to on this framebuffer by leaving
>>> +          * GL_FRAMEBUFFER_SRGB in place.
>>> +          */
>>>  	 if (ctx->Extensions.EXT_texture_sRGB_decode) {
>>>  	    _mesa_TexParameteri(target, GL_TEXTURE_SRGB_DECODE_EXT,
>>> -				GL_SKIP_DECODE_EXT);
>>> +				GL_DECODE_EXT);
>>>  	 }
>>
>> Wait, won't this "break" sRGB -> sRGB blits?
>>
>> I guess I'd expect that:
>>
>> 1. Blitting from a RGBA source to an RGBA destination should be an exact
>> copy: no decoding/encoding should occur, regardless of any enable bits.
>>
>> 2. Blitting from an sRGBA source to an sRGBA destination should also be
>> an exact copy---both buffers should then contain the same pixel values.
>>
>> These seem pretty clear, and also match Mesa's current behavior.
>>
>> With your patch, if an application had set GL_FRAMEBUFFER_SRGB_EXT to
>> GL_TRUE, then I believe case 2 would result in the two buffers having
>> different values: non-decoded source values would get encoded.
>>
>> One could argue that such an application is being stupid, but honestly
>> as confusing as these specs are, I don't know what stupid is anymore.
>>
>> I feel like blits should follow these rules for mixed formats:
>>
>> 3. Blitting from an sRGBA source to an RGBA destination should
>> optionally decode the values based on whether the application has set
>> GL_TEXTURE_SRGB_DECODE_EXT to GL_DECODE_EXT or GL_SKIP_DECODE_EXT.  The
>> other knob, GL_FRAMEBUFFER_SRGB_EXT, should be ignored since it controls
>> writes and the destination is an ordinary format (RGBA).
>>
>> 4. Blitting from an RGBA source to an sRGBA destination should
>> optionally encode the values based on whether the application has set
>> GL_FRAMEBUFFER_SRGB_EXT to true or false.  The other knob,
>> GL_TEXTURE_SRGB_DECODE_EXT, should be ignored since it controls
>> reads/texturing and the source is not an sRGB format.
>>
>> These four rules allow you to copy SRGB->SRGB and RGB->RGB unmodified,
>> while also allowing you to decode SRGB values into a linear buffer, or
>> encode linear values into an SRGB buffer.  That seems like it covers all
>> the cases anyone would want to do.
>>
>>> -         if (ctx->Extensions.EXT_framebuffer_sRGB) {
>>> -            _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_FALSE);
>>> -         }
>>>  
>>>           _mesa_TexEnvi(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_REPLACE);
>>>           _mesa_set_enable(ctx, target, GL_TRUE);
>>> @@ -1463,9 +1462,6 @@ blitframebuffer_texture(struct gl_context *ctx,
>>>  	 if (ctx->Extensions.EXT_texture_sRGB_decode) {
>>>  	    _mesa_TexParameteri(target, GL_TEXTURE_SRGB_DECODE_EXT, srgbSave);
>>>  	 }
>>> -	 if (ctx->Extensions.EXT_framebuffer_sRGB && fbo_srgb_save) {
>>> -	    _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_TRUE);
>>> -	 }
>>>  
>>>           /* Done with color buffer */
>>>           mask &= ~GL_COLOR_BUFFER_BIT;
>>>
>>
>> Regardless of what we decide to do here, I also wonder if any updates
>> are necessary to the i965 BLT or blorp code.  meta blits are the last
>> resort in i965, and I definitely saw different behavior in L4D2 by
>> commenting out the BLT and blorp and only using meta.
>>
>> We should at least make sure that BLT/blorp either follow the same rules
>> as meta, or refuse to blit in these cases.
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev