[Mesa-dev] [PATCH 1/2] i965: Make sRGB-capable framebuffers by default.

[Ian Romanick]

On 02/22/2013 12:00 PM, Eric Anholt wrote:
> The GLX extension lets you expose visuals that explicitly guarantee you
> that the GL_FRAMEBUFFER_SRGB_CAPABLE flag will be set, but we can set
> the flag even while the visual doesn't provide the guarantee.  This
> appears to be consistent with other implementations, as we've seen
> several apps now that don't require an srgb visual and assume sRGB will
> work without checking the GL_FRAMEBUFFER_SRGB_CAPABLE flag.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=55783
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=60633

I don't see any regressions on gles3conform from this.  Looks good to 
me.  The series is

Reviewed-and-tested-by: Ian Romanick <ian.d.romanick at intel.com>

> ---
>   src/mesa/drivers/dri/intel/intel_context.c |   55 +++++++++++++++++++++++++++-
>   src/mesa/drivers/dri/intel/intel_screen.c  |   11 +++++-
>   2 files changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_context.c b/src/mesa/drivers/dri/intel/intel_context.c
> index 9bb7156..488cf9b 100644
> --- a/src/mesa/drivers/dri/intel/intel_context.c
> +++ b/src/mesa/drivers/dri/intel/intel_context.c
> @@ -906,6 +906,55 @@ intelUnbindContext(__DRIcontext * driContextPriv)
>      return true;
>   }
>
> +/**
> + * Fixes up the context for GLES23 with our default-to-sRGB-capable behavior
> + * on window system framebuffers.
> + *
> + * Desktop GL fairly reasonable in its handling of sRGB: You can ask if your
> + * renderbuffer can do sRGB encode, and you can flip a switch that does sRGB
> + * encode if the renderbuffer can handle it.  You can ask specifically for a
> + * visual where you're guaranteed to be capable, but it turns out that
> + * everyone just makes all their ARGB8888 visuals capable and doesn't offer
> + * incapable ones, becuase there's no difference between the two in resources
> + * used.  Applications thus get built that accidentally rely on the default
> + * visual choice being sRGB, so we make ours sRGB capable.  Everything sounds
> + * great...
> + *
> + * But for GLES2/3, they decided that it was silly to not turn on sRGB encode
> + * for sRGB renderbuffers you made with the GL_EXT_texture_sRGB equivalent.
> + * So they removed the enable knob and made it "if the renderbuffer is sRGB
> + * capable, do sRGB encode".  Then, for your window system renderbuffers, you
> + * can ask for sRGB visuals and get sRGB encode, or not ask for sRGB visuals
> + * and get no sRGB encode (assuming that both kinds of visual are available).
> + * Thus our choice to support sRGB by default on our visuals for desktop would
> + * result in broken rendering of GLES apps that aren't expecting sRGB encode.
> + *
> + * Unfortunately, renderbuffer setup happens before a context is created.  So
> + * in intel_screen.c we always set up sRGB, and here, if you're a GLES2/3
> + * context (without an sRGB visual, though we don't have sRGB visuals exposed
> + * yet), we go turn that back off before anyone finds out.
> + */
> +static void
> +intel_gles3_srgb_workaround(struct intel_context *intel,
> +                            struct gl_framebuffer *fb)
> +{
> +   struct gl_context *ctx = &intel->ctx;
> +
> +   if (_mesa_is_desktop_gl(ctx) || !fb->Visual.sRGBCapable)
> +      return;
> +
> +   /* Some day when we support the sRGB capable bit on visuals available for
> +    * GLES, we'll need to respect that and not disable things here.
> +    */
> +   fb->Visual.sRGBCapable = false;
> +   for (int i = 0; i < BUFFER_COUNT; i++) {
> +      if (fb->Attachment[i].Renderbuffer &&
> +          fb->Attachment[i].Renderbuffer->Format == MESA_FORMAT_SARGB8) {
> +         fb->Attachment[i].Renderbuffer->Format = MESA_FORMAT_ARGB8888;
> +      }
> +   }
> +}
> +
>   GLboolean
>   intelMakeCurrent(__DRIcontext * driContextPriv,
>                    __DRIdrawable * driDrawPriv,
> @@ -928,6 +977,7 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
>      }
>
>      if (driContextPriv) {
> +      struct gl_context *ctx = &intel->ctx;
>         struct gl_framebuffer *fb, *readFb;
>
>         if (driDrawPriv == NULL && driReadPriv == NULL) {
> @@ -942,7 +992,10 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
>
>         intel_prepare_render(intel);
>         _mesa_make_current(&intel->ctx, fb, readFb);
> -
> +
> +      intel_gles3_srgb_workaround(intel, ctx->WinSysDrawBuffer);
> +      intel_gles3_srgb_workaround(intel, ctx->WinSysReadBuffer);
> +
>         /* We do this in intel_prepare_render() too, but intel->ctx.DrawBuffer
>          * is NULL at that point.  We can't call _mesa_makecurrent()
>          * first, since we need the buffer size for the initial
> diff --git a/src/mesa/drivers/dri/intel/intel_screen.c b/src/mesa/drivers/dri/intel/intel_screen.c
> index d223a0b..f76e6f9 100644
> --- a/src/mesa/drivers/dri/intel/intel_screen.c
> +++ b/src/mesa/drivers/dri/intel/intel_screen.c
> @@ -795,8 +795,15 @@ intelCreateBuffer(__DRIscreen * driScrnPriv,
>         rgbFormat = MESA_FORMAT_SARGB8;
>      else if (mesaVis->alphaBits == 0)
>         rgbFormat = MESA_FORMAT_XRGB8888;
> -   else
> -      rgbFormat = MESA_FORMAT_ARGB8888;
> +   else {
> +      if (screen->gen >= 4) {
> +         rgbFormat = MESA_FORMAT_SARGB8;
> +         fb->Visual.sRGBCapable = true;
> +      } else {
> +         rgbFormat = MESA_FORMAT_ARGB8888;
> +      }
> +
> +   }
>
>      /* setup the hardware-based renderbuffers */
>      rb = intel_create_renderbuffer(rgbFormat, num_samples);
>