[Mesa-dev] [PATCH v2] dri/i965: fix incorrect rgbFormat in intelCreateBuffer().

[Haixia Shi]

Ilia: please review patch v4. I have added a separate patch to fix the
GLES3 sRGB workaround, which fixes the colorspace issue.

On Tue, Apr 5, 2016 at 5:20 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:

> Sounds right. That's glEnable/glDisable(GL_FRAMEBUFFER_SRGB). It
> defaults to off for desktop GL, and on for GL ES (why? no clue.)
>
> On Tue, Apr 5, 2016 at 8:11 PM, Haixia Shi <hshi at chromium.org> wrote:
> > Are you referring to the following check
> >
> > mesa_format
> > _mesa_get_render_format(const struct gl_context *ctx, mesa_format format)
> > {
> >    if (ctx->Color.sRGBEnabled)
> >       return format;
> >    else
> >       return _mesa_get_srgb_format_linear(format);
> > }
> >
> > So if the platform sets ctx->Color.sRGBEnabled = false then I should not
> > have this problem? Thanks
> >
> > On Tue, Apr 5, 2016 at 2:04 PM, Ilia Mirkin <imirkin at alum.mit.edu>
> wrote:
> >>
> >> On Tue, Apr 5, 2016 at 4:57 PM, Haixia Shi <hshi at chromium.org> wrote:
> >> > CC Stephane
> >> >
> >> > In my opinion we should not assume sRGB-capable when alpha is
> requested,
> >> > because UNORM is the more common use case. I actually have a use case
> >> > where
> >> > alpha is requested but not sRGBCapable, and if we assume SRGB by
> default
> >> > then we will get really white-washed faded color.
> >>
> >> That most likely means that your application is buggy. Even though the
> >> SRGB format is returned internally, that doesn't mean that SRGB
> >> encoding/decoding is actually enabled. Just provides the option for it
> >> to be used.
> >>
> >> >
> >> > On Tue, Apr 5, 2016 at 1:52 PM, Ilia Mirkin <imirkin at alum.mit.edu>
> >> > wrote:
> >> >>
> >> >> On Tue, Apr 5, 2016 at 4:33 PM, Haixia Shi <hshi at chromium.org>
> wrote:
> >> >> > It is incorrect to assume that pixel format is always in BGR byte
> >> >> > order.
> >> >> > We need to check bitmask parameters (such as |redMask|) to
> determine
> >> >> > whether
> >> >> > the RGB or BGR byte order is requested.
> >> >> >
> >> >> > Furthermore when parameter |sRGBCapable| is set to false, we should
> >> >> > be
> >> >> > using
> >> >> > UNORM format by default.
> >> >>
> >> >> Unfortunately none of the visuals are exported with that flag, so it
> >> >> will always be false in practice. The current logic is also very
> >> >> fragile -- you only get a sRGB-capable fb if you happen to request
> >> >> alpha.
> >> >>
> >> >> >
> >> >> > v2: reformat code to stay within 80 character per line limit.
> >> >> >
> >> >> > Signed-off-by: Haixia Shi <hshi at chromium.org>
> >> >> > Reviewed-by: Stéphane Marchesin <marcheu at chromium.org>
> >> >> > Cc: kenneth.w.graunke at intel.com
> >> >> >
> >> >> > Change-Id: Ib75087aef1fbfb51baa72517207fed410dcd7b1e
> >> >> > ---
> >> >> >  src/mesa/drivers/dri/i965/intel_screen.c | 20 ++++++++++++--------
> >> >> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >> >> >
> >> >> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> >> >> > b/src/mesa/drivers/dri/i965/intel_screen.c
> >> >> > index c6eb50a..a27d7ee 100644
> >> >> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> >> >> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> >> >> > @@ -1000,15 +1000,19 @@ intelCreateBuffer(__DRIscreen *
> driScrnPriv,
> >> >> >        fb->Visual.samples = num_samples;
> >> >> >     }
> >> >> >
> >> >> > -   if (mesaVis->redBits == 5)
> >> >> > -      rgbFormat = MESA_FORMAT_B5G6R5_UNORM;
> >> >> > -   else if (mesaVis->sRGBCapable)
> >> >> > -      rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
> >> >> > -   else if (mesaVis->alphaBits == 0)
> >> >> > -      rgbFormat = MESA_FORMAT_B8G8R8X8_UNORM;
> >> >> > -   else {
> >> >> > -      rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
> >> >> > +   if (mesaVis->redBits == 5) {
> >> >> > +      rgbFormat = mesaVis->redMask == 0x1f ?
> >> >> > MESA_FORMAT_R5G6B5_UNORM
> >> >> > +                                           :
> >> >> > MESA_FORMAT_B5G6R5_UNORM;
> >> >> > +   } else if (mesaVis->alphaBits == 0) {
> >> >> > +      rgbFormat = mesaVis->redMask == 0xff ?
> >> >> > MESA_FORMAT_R8G8B8X8_UNORM
> >> >> > +                                           :
> >> >> > MESA_FORMAT_B8G8R8X8_UNORM;
> >> >> > +   } else if (mesaVis->sRGBCapable) {
> >> >> > +      rgbFormat = mesaVis->redMask == 0xff ?
> >> >> > MESA_FORMAT_R8G8B8A8_SRGB
> >> >> > +                                           :
> >> >> > MESA_FORMAT_B8G8R8A8_SRGB;
> >> >> >        fb->Visual.sRGBCapable = true;
> >> >> > +   } else {
> >> >> > +      rgbFormat = mesaVis->redMask == 0xff ?
> >> >> > MESA_FORMAT_R8G8B8A8_UNORM
> >> >> > +                                           :
> >> >> > MESA_FORMAT_B8G8R8A8_UNORM;
> >> >> >     }
> >> >> >
> >> >> >     /* setup the hardware-based renderbuffers */
> >> >> > --
> >> >> > 2.8.0.rc3.226.g39d4020
> >> >> >
> >> >> > _______________________________________________
> >> >> > mesa-dev mailing list
> >> >> > mesa-dev at lists.freedesktop.org
> >> >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160406/63429630/attachment.html>