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

[Kenneth Graunke]

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.
> 
> 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.