Re: [Mesa-dev] [PATCH 1/2] dri/i965: extend GLES3 sRGB workaround to cover all byte orders

[Jason Ekstrand]

On Wed, Apr 6, 2016 at 10:35 PM, Haixia Shi  wrote:

> I am afraid that I am not the original author of the
> gles3_srgb_workaround, and from the comments it appears to only apply for
> argb formats, so I am not sure if extending it to all other formats would
> cause behavioral regression.
> On Apr 6, 2016 10:13 PM, "Jason Ekstrand"  wrote:
>
>>
>> On Apr 6, 2016 7:23 PM, "Kenneth Graunke"  wrote:
>> >
>> > On Wednesday, April 6, 2016 4:43:39 PM PDT Haixia Shi wrote:
>> > > It is incorrect to assume BGRA byte order for the GLES3 sRGB
>> workaround.
>> > >
>> > > Signed-off-by: Haixia Shi 
>> > > Reviewed-by: Stéphane Marchesin 
>> > > Cc: kenneth.w.grau...@intel.com
>> > >
>> > > Change-Id: I5a081d7eaa7544afff0e7874cffef80d3f69a401
>> > > ---
>> > >  src/mesa/drivers/dri/i965/brw_context.c | 18 --
>> > >  1 file changed, 16 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>> b/src/mesa/drivers/dri/
>> > i965/brw_context.c
>> > > index 2d480d0..cebfbda 100644
>> > > --- a/src/mesa/drivers/dri/i965/brw_context.c
>> > > +++ b/src/mesa/drivers/dri/i965/brw_context.c
>> > > @@ -1151,9 +1151,23 @@ intel_gles3_srgb_workaround(struct brw_context
>> *brw,
>> > >  */
>> > > fb->Visual.sRGBCapable = false;
>> > > for (int i = 0; i < BUFFER_COUNT; i++) {
>> > > -  if (fb->Attachment[i].Renderbuffer &&
>> > > -  fb->Attachment[i].Renderbuffer->Format ==
>> > MESA_FORMAT_B8G8R8A8_SRGB) {
>> > > +  if (!fb->Attachment[i].Renderbuffer)
>> > > + continue;
>> > > +  switch (fb->Attachment[i].Renderbuffer->Format) {
>> > > +  case MESA_FORMAT_A8B8G8R8_SRGB:
>> > > + fb->Attachment[i].Renderbuffer->Format =
>> > MESA_FORMAT_A8B8G8R8_UNORM;
>> > > + break;
>> > > +  case MESA_FORMAT_B8G8R8A8_SRGB:
>> > >   fb->Attachment[i].Renderbuffer->Format =
>> > MESA_FORMAT_B8G8R8A8_UNORM;
>> > > + break;
>> > > +  case MESA_FORMAT_A8R8G8B8_SRGB:
>> > > + fb->Attachment[i].Renderbuffer->Format =
>> > MESA_FORMAT_A8R8G8B8_UNORM;
>> > > + break;
>> > > +  case MESA_FORMAT_R8G8B8A8_SRGB:
>> > > + fb->Attachment[i].Renderbuffer->Format =
>> > MESA_FORMAT_R8G8B8A8_UNORM;
>> > > + break;
>> > > +  default:
>> > > + break;
>> > >}
>> > > }
>> > >  }
>> > >
>> >
>> > Why don't we simply do:
>> >
>> >struct gl_renderbuffer *rb = fb->Attachment[i].Renderbuffer;
>> >rb->Format = _mesa_get_srgb_format_linear(rb->Format);
>> >
>> > This would handle far more formats than we need to, but that shouldn't
>> > be a problem.
>>
>> I'll second that.
>>
>
Look at the comment right above the function in question, I think Ken's
suggest is exactly the right thing to do.  There's nothing involved here
that should be -specific.
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] dri/i965: extend GLES3 sRGB workaround to cover all byte orders

[Haixia Shi]

I am afraid that I am not the original author of the gles3_srgb_workaround,
and from the comments it appears to only apply for argb formats, so I am
not sure if extending it to all other formats would cause behavioral
regression.
On Apr 6, 2016 10:13 PM, "Jason Ekstrand"  wrote:

>
> On Apr 6, 2016 7:23 PM, "Kenneth Graunke"  wrote:
> >
> > On Wednesday, April 6, 2016 4:43:39 PM PDT Haixia Shi wrote:
> > > It is incorrect to assume BGRA byte order for the GLES3 sRGB
> workaround.
> > >
> > > Signed-off-by: Haixia Shi 
> > > Reviewed-by: Stéphane Marchesin 
> > > Cc: kenneth.w.grau...@intel.com
> > >
> > > Change-Id: I5a081d7eaa7544afff0e7874cffef80d3f69a401
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_context.c | 18 --
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/
> > i965/brw_context.c
> > > index 2d480d0..cebfbda 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > > @@ -1151,9 +1151,23 @@ intel_gles3_srgb_workaround(struct brw_context
> *brw,
> > >  */
> > > fb->Visual.sRGBCapable = false;
> > > for (int i = 0; i < BUFFER_COUNT; i++) {
> > > -  if (fb->Attachment[i].Renderbuffer &&
> > > -  fb->Attachment[i].Renderbuffer->Format ==
> > MESA_FORMAT_B8G8R8A8_SRGB) {
> > > +  if (!fb->Attachment[i].Renderbuffer)
> > > + continue;
> > > +  switch (fb->Attachment[i].Renderbuffer->Format) {
> > > +  case MESA_FORMAT_A8B8G8R8_SRGB:
> > > + fb->Attachment[i].Renderbuffer->Format =
> > MESA_FORMAT_A8B8G8R8_UNORM;
> > > + break;
> > > +  case MESA_FORMAT_B8G8R8A8_SRGB:
> > >   fb->Attachment[i].Renderbuffer->Format =
> > MESA_FORMAT_B8G8R8A8_UNORM;
> > > + break;
> > > +  case MESA_FORMAT_A8R8G8B8_SRGB:
> > > + fb->Attachment[i].Renderbuffer->Format =
> > MESA_FORMAT_A8R8G8B8_UNORM;
> > > + break;
> > > +  case MESA_FORMAT_R8G8B8A8_SRGB:
> > > + fb->Attachment[i].Renderbuffer->Format =
> > MESA_FORMAT_R8G8B8A8_UNORM;
> > > + break;
> > > +  default:
> > > + break;
> > >}
> > > }
> > >  }
> > >
> >
> > Why don't we simply do:
> >
> >struct gl_renderbuffer *rb = fb->Attachment[i].Renderbuffer;
> >rb->Format = _mesa_get_srgb_format_linear(rb->Format);
> >
> > This would handle far more formats than we need to, but that shouldn't
> > be a problem.
>
> I'll second that.
>
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] dri/i965: extend GLES3 sRGB workaround to cover all byte orders

[Jason Ekstrand]

On Apr 6, 2016 7:23 PM, "Kenneth Graunke"  wrote:
>
> On Wednesday, April 6, 2016 4:43:39 PM PDT Haixia Shi wrote:
> > It is incorrect to assume BGRA byte order for the GLES3 sRGB workaround.
> >
> > Signed-off-by: Haixia Shi 
> > Reviewed-by: Stéphane Marchesin 
> > Cc: kenneth.w.grau...@intel.com
> >
> > Change-Id: I5a081d7eaa7544afff0e7874cffef80d3f69a401
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.c | 18 --
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
b/src/mesa/drivers/dri/
> i965/brw_context.c
> > index 2d480d0..cebfbda 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -1151,9 +1151,23 @@ intel_gles3_srgb_workaround(struct brw_context
*brw,
> >  */
> > fb->Visual.sRGBCapable = false;
> > for (int i = 0; i < BUFFER_COUNT; i++) {
> > -  if (fb->Attachment[i].Renderbuffer &&
> > -  fb->Attachment[i].Renderbuffer->Format ==
> MESA_FORMAT_B8G8R8A8_SRGB) {
> > +  if (!fb->Attachment[i].Renderbuffer)
> > + continue;
> > +  switch (fb->Attachment[i].Renderbuffer->Format) {
> > +  case MESA_FORMAT_A8B8G8R8_SRGB:
> > + fb->Attachment[i].Renderbuffer->Format =
> MESA_FORMAT_A8B8G8R8_UNORM;
> > + break;
> > +  case MESA_FORMAT_B8G8R8A8_SRGB:
> >   fb->Attachment[i].Renderbuffer->Format =
> MESA_FORMAT_B8G8R8A8_UNORM;
> > + break;
> > +  case MESA_FORMAT_A8R8G8B8_SRGB:
> > + fb->Attachment[i].Renderbuffer->Format =
> MESA_FORMAT_A8R8G8B8_UNORM;
> > + break;
> > +  case MESA_FORMAT_R8G8B8A8_SRGB:
> > + fb->Attachment[i].Renderbuffer->Format =
> MESA_FORMAT_R8G8B8A8_UNORM;
> > + break;
> > +  default:
> > + break;
> >}
> > }
> >  }
> >
>
> Why don't we simply do:
>
>struct gl_renderbuffer *rb = fb->Attachment[i].Renderbuffer;
>rb->Format = _mesa_get_srgb_format_linear(rb->Format);
>
> This would handle far more formats than we need to, but that shouldn't
> be a problem.

I'll second that.

> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] dri/i965: extend GLES3 sRGB workaround to cover all byte orders

[Kenneth Graunke]

On Wednesday, April 6, 2016 4:43:39 PM PDT Haixia Shi wrote:
> It is incorrect to assume BGRA byte order for the GLES3 sRGB workaround.
> 
> Signed-off-by: Haixia Shi 
> Reviewed-by: Stéphane Marchesin 
> Cc: kenneth.w.grau...@intel.com
> 
> Change-Id: I5a081d7eaa7544afff0e7874cffef80d3f69a401
> ---
>  src/mesa/drivers/dri/i965/brw_context.c | 18 --
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/
i965/brw_context.c
> index 2d480d0..cebfbda 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1151,9 +1151,23 @@ intel_gles3_srgb_workaround(struct brw_context *brw,
>  */
> fb->Visual.sRGBCapable = false;
> for (int i = 0; i < BUFFER_COUNT; i++) {
> -  if (fb->Attachment[i].Renderbuffer &&
> -  fb->Attachment[i].Renderbuffer->Format == 
MESA_FORMAT_B8G8R8A8_SRGB) {
> +  if (!fb->Attachment[i].Renderbuffer)
> + continue;
> +  switch (fb->Attachment[i].Renderbuffer->Format) {
> +  case MESA_FORMAT_A8B8G8R8_SRGB:
> + fb->Attachment[i].Renderbuffer->Format = 
MESA_FORMAT_A8B8G8R8_UNORM;
> + break;
> +  case MESA_FORMAT_B8G8R8A8_SRGB:
>   fb->Attachment[i].Renderbuffer->Format = 
MESA_FORMAT_B8G8R8A8_UNORM;
> + break;
> +  case MESA_FORMAT_A8R8G8B8_SRGB:
> + fb->Attachment[i].Renderbuffer->Format = 
MESA_FORMAT_A8R8G8B8_UNORM;
> + break;
> +  case MESA_FORMAT_R8G8B8A8_SRGB:
> + fb->Attachment[i].Renderbuffer->Format = 
MESA_FORMAT_R8G8B8A8_UNORM;
> + break;
> +  default:
> + break;
>}
> }
>  }
> 

Why don't we simply do:

   struct gl_renderbuffer *rb = fb->Attachment[i].Renderbuffer;
   rb->Format = _mesa_get_srgb_format_linear(rb->Format);

This would handle far more formats than we need to, but that shouldn't
be a problem.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] dri/i965: extend GLES3 sRGB workaround to cover all byte orders

[Haixia Shi]

It is incorrect to assume BGRA byte order for the GLES3 sRGB workaround.

Signed-off-by: Haixia Shi 
Reviewed-by: Stéphane Marchesin 
Cc: kenneth.w.grau...@intel.com

Change-Id: I5a081d7eaa7544afff0e7874cffef80d3f69a401
---
 src/mesa/drivers/dri/i965/brw_context.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 2d480d0..cebfbda 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1151,9 +1151,23 @@ intel_gles3_srgb_workaround(struct brw_context *brw,
 */
fb->Visual.sRGBCapable = false;
for (int i = 0; i < BUFFER_COUNT; i++) {
-  if (fb->Attachment[i].Renderbuffer &&
-  fb->Attachment[i].Renderbuffer->Format == MESA_FORMAT_B8G8R8A8_SRGB) 
{
+  if (!fb->Attachment[i].Renderbuffer)
+ continue;
+  switch (fb->Attachment[i].Renderbuffer->Format) {
+  case MESA_FORMAT_A8B8G8R8_SRGB:
+ fb->Attachment[i].Renderbuffer->Format = MESA_FORMAT_A8B8G8R8_UNORM;
+ break;
+  case MESA_FORMAT_B8G8R8A8_SRGB:
  fb->Attachment[i].Renderbuffer->Format = MESA_FORMAT_B8G8R8A8_UNORM;
+ break;
+  case MESA_FORMAT_A8R8G8B8_SRGB:
+ fb->Attachment[i].Renderbuffer->Format = MESA_FORMAT_A8R8G8B8_UNORM;
+ break;
+  case MESA_FORMAT_R8G8B8A8_SRGB:
+ fb->Attachment[i].Renderbuffer->Format = MESA_FORMAT_R8G8B8A8_UNORM;
+ break;
+  default:
+ break;
   }
}
 }
-- 
2.8.0.rc3.226.g39d4020

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev