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

[Christoph Bumiller]

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.

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