Re: [Mesa-dev] [PATCH v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

2016-11-15 Thread Eduardo Lima Mitev 
On 11/15/2016 10:58 AM, Kenneth Graunke wrote:
> On Tuesday, November 15, 2016 9:27:00 AM PST Eduardo Lima Mitev wrote:
>> On 11/15/2016 12:25 AM, Kenneth Graunke wrote:
>>> From: Eduardo Lima Mitev 
>>>
>>> This option was being ignored when packing compressed 3D and cube textures.
>>>
>>> Fixes CTS test (on gen8+):
>>> * GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore
>>>
>>> v2: Drop API checks.
>>> v3 (Ken): Just apply the existing code in more cases.
>>> ---
>>>  src/mesa/drivers/common/meta.c | 20 +---
>>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> Hey Eduardo,
>>>
>>> It looks like the existing code already tries to handle SkipImages - but
>>> we weren't applying it for 3D and cubemap textures.  I found a spec quote
>>> in the narrative for GetTexImage that indicates we need to do it for those
>>> as well.
>>>
>>> What do you think of this version?  I preserved your authorship as I wanted
>>> you to get the credit for this bugfix - I just typed it up so that I could
>>> make sure it actually fixed the test, and since I had it typed up, I figured
>>> I'd send it out to save you some time...
>>>
>>
>> Hi Kenneth,
>>
>> Great, this is the correct solution. I tried to make my original patch 
>> work with _mesa_image_address3d() for all targets, as yours, but I 
>> missed resetting SkipPixels and SkipRows for TEXTURE_3D, so it was 
>> regressing some subcases.
>>
>> Patch is:
>>
>> Reviewed-by: Eduardo Lima Mitev 
>>
>> I don't mind you taking authorship. Knowing the correct solution is way 
>> more useful to me :). If you want, take the authorship, put my R-b and 
>> push; otherwise let me know and I'll put your R-b and push it myself.
>>
>> Thanks!
>>
>> Eduardo
> 
> Reviewed-by: Kenneth Graunke 
> 
> Go ahead and push it :) Thanks!
> 

Pushed, thanks Ken!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

2016-11-15 Thread Eduardo Lima Mitev 
On 11/15/2016 11:03 AM, Nicolai Hähnle wrote:
> On 15.11.2016 09:27, Eduardo Lima Mitev wrote:
>> On 11/15/2016 12:25 AM, Kenneth Graunke wrote:
>>> From: Eduardo Lima Mitev 
>>>
>>> This option was being ignored when packing compressed 3D and cube
>>> textures.
>>>
>>> Fixes CTS test (on gen8+):
>>> * GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore
>>>
>>> v2: Drop API checks.
>>> v3 (Ken): Just apply the existing code in more cases.
>>> ---
>>>  src/mesa/drivers/common/meta.c | 20 +---
>>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> Hey Eduardo,
>>>
>>> It looks like the existing code already tries to handle SkipImages - but
>>> we weren't applying it for 3D and cubemap textures.  I found a spec
>>> quote
>>> in the narrative for GetTexImage that indicates we need to do it for
>>> those
>>> as well.
>>>
>>> What do you think of this version?  I preserved your authorship as I
>>> wanted
>>> you to get the credit for this bugfix - I just typed it up so that I
>>> could
>>> make sure it actually fixed the test, and since I had it typed up, I
>>> figured
>>> I'd send it out to save you some time...
>>>
>>
>> Hi Kenneth,
>>
>> Great, this is the correct solution. I tried to make my original patch
>> work with _mesa_image_address3d() for all targets, as yours, but I
>> missed resetting SkipPixels and SkipRows for TEXTURE_3D, so it was
>> regressing some subcases.
> 
> For what it's worth, there's also _mesa_image_address which takes a
> dimensions parameter and automatically applies / doesn't apply
> SkipImages. But since you do manual fixups of SkipPixels/SkipRows
> anyway, it might not be the right thing here.
>

Yes, we could use _mesa_image_address() directly, but I think it is more
legible to call mesa_image_address3d(), because it is consistent with
the purpose of this code-path and the comment in it, being for
dimensions >2.

Thanks,
Eduardo

> Cheers,
> Nicolai
> 
>>
>> Patch is:
>>
>> Reviewed-by: Eduardo Lima Mitev 
>>
>> I don't mind you taking authorship. Knowing the correct solution is way
>> more useful to me :). If you want, take the authorship, put my R-b and
>> push; otherwise let me know and I'll put your R-b and push it myself.
>>
>> Thanks!
>>
>> Eduardo
>>
>>>  --Ken
>>>
>>> diff --git a/src/mesa/drivers/common/meta.c
>>> b/src/mesa/drivers/common/meta.c
>>> index 5ab1e6c..99c85cf 100644
>>> --- a/src/mesa/drivers/common/meta.c
>>> +++ b/src/mesa/drivers/common/meta.c
>>> @@ -3243,8 +3243,20 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
>>>
>>>for (slice = 0; slice < depth; slice++) {
>>>   void *dst;
>>> - if (texImage->TexObject->Target == GL_TEXTURE_2D_ARRAY
>>> - || texImage->TexObject->Target ==
>>> GL_TEXTURE_CUBE_MAP_ARRAY) {
>>> + /* Section 8.11.4 (Texture Image Queries) of the GL 4.5 spec
>>> says:
>>> +  *
>>> +  *"For three-dimensional, two-dimensional array, cube
>>> map array,
>>> +  * and cube map textures pixel storage operations are
>>> applied as
>>> +  * if the image were two-dimensional, except that the
>>> additional
>>> +  * pixel storage state values PACK_IMAGE_HEIGHT and
>>> +  * PACK_SKIP_IMAGES are applied. The correspondence of
>>> texels to
>>> +  * memory locations is as defined for TexImage3D in
>>> section 8.5."
>>> +  */
>>> + switch (texImage->TexObject->Target) {
>>> + case GL_TEXTURE_3D:
>>> + case GL_TEXTURE_2D_ARRAY:
>>> + case GL_TEXTURE_CUBE_MAP:
>>> + case GL_TEXTURE_CUBE_MAP_ARRAY: {
>>>  /* Setup pixel packing.  SkipPixels and SkipRows will be
>>> applied
>>>   * in the decompress_texture_image() function's call to
>>>   * glReadPixels but we need to compute the dest slice's
>>> address
>>> @@ -3255,9 +3267,11 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
>>>  packing.SkipRows = 0;
>>>  dst = _mesa_image_address3d(, pixels, width,
>>> height,
>>>  format, type, slice, 0, 0);
>>> +break;
>>>   }
>>> - else {
>>> + default:
>>>  dst = pixels;
>>> +break;
>>>   }
>>>   result = decompress_texture_image(ctx, texImage, slice,
>>> xoffset, yoffset, width,
>>> height,
>>>
>>
>> ___
>> 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 v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

2016-11-15 Thread Nicolai Hähnle 

On 15.11.2016 09:27, Eduardo Lima Mitev wrote:

On 11/15/2016 12:25 AM, Kenneth Graunke wrote:

From: Eduardo Lima Mitev 

This option was being ignored when packing compressed 3D and cube
textures.

Fixes CTS test (on gen8+):
* GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore

v2: Drop API checks.
v3 (Ken): Just apply the existing code in more cases.
---
 src/mesa/drivers/common/meta.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

Hey Eduardo,

It looks like the existing code already tries to handle SkipImages - but
we weren't applying it for 3D and cubemap textures.  I found a spec quote
in the narrative for GetTexImage that indicates we need to do it for
those
as well.

What do you think of this version?  I preserved your authorship as I
wanted
you to get the credit for this bugfix - I just typed it up so that I
could
make sure it actually fixed the test, and since I had it typed up, I
figured
I'd send it out to save you some time...



Hi Kenneth,

Great, this is the correct solution. I tried to make my original patch
work with _mesa_image_address3d() for all targets, as yours, but I
missed resetting SkipPixels and SkipRows for TEXTURE_3D, so it was
regressing some subcases.


For what it's worth, there's also _mesa_image_address which takes a 
dimensions parameter and automatically applies / doesn't apply 
SkipImages. But since you do manual fixups of SkipPixels/SkipRows 
anyway, it might not be the right thing here.


Cheers,
Nicolai



Patch is:

Reviewed-by: Eduardo Lima Mitev 

I don't mind you taking authorship. Knowing the correct solution is way
more useful to me :). If you want, take the authorship, put my R-b and
push; otherwise let me know and I'll put your R-b and push it myself.

Thanks!

Eduardo


 --Ken

diff --git a/src/mesa/drivers/common/meta.c
b/src/mesa/drivers/common/meta.c
index 5ab1e6c..99c85cf 100644
--- a/src/mesa/drivers/common/meta.c
+++ b/src/mesa/drivers/common/meta.c
@@ -3243,8 +3243,20 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,

   for (slice = 0; slice < depth; slice++) {
  void *dst;
- if (texImage->TexObject->Target == GL_TEXTURE_2D_ARRAY
- || texImage->TexObject->Target ==
GL_TEXTURE_CUBE_MAP_ARRAY) {
+ /* Section 8.11.4 (Texture Image Queries) of the GL 4.5 spec
says:
+  *
+  *"For three-dimensional, two-dimensional array, cube
map array,
+  * and cube map textures pixel storage operations are
applied as
+  * if the image were two-dimensional, except that the
additional
+  * pixel storage state values PACK_IMAGE_HEIGHT and
+  * PACK_SKIP_IMAGES are applied. The correspondence of
texels to
+  * memory locations is as defined for TexImage3D in
section 8.5."
+  */
+ switch (texImage->TexObject->Target) {
+ case GL_TEXTURE_3D:
+ case GL_TEXTURE_2D_ARRAY:
+ case GL_TEXTURE_CUBE_MAP:
+ case GL_TEXTURE_CUBE_MAP_ARRAY: {
 /* Setup pixel packing.  SkipPixels and SkipRows will be
applied
  * in the decompress_texture_image() function's call to
  * glReadPixels but we need to compute the dest slice's
address
@@ -3255,9 +3267,11 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
 packing.SkipRows = 0;
 dst = _mesa_image_address3d(, pixels, width, height,
 format, type, slice, 0, 0);
+break;
  }
- else {
+ default:
 dst = pixels;
+break;
  }
  result = decompress_texture_image(ctx, texImage, slice,
xoffset, yoffset, width,
height,



___
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 v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

2016-11-15 Thread Kenneth Graunke 
On Tuesday, November 15, 2016 9:27:00 AM PST Eduardo Lima Mitev wrote:
> On 11/15/2016 12:25 AM, Kenneth Graunke wrote:
> > From: Eduardo Lima Mitev 
> >
> > This option was being ignored when packing compressed 3D and cube textures.
> >
> > Fixes CTS test (on gen8+):
> > * GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore
> >
> > v2: Drop API checks.
> > v3 (Ken): Just apply the existing code in more cases.
> > ---
> >  src/mesa/drivers/common/meta.c | 20 +---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > Hey Eduardo,
> >
> > It looks like the existing code already tries to handle SkipImages - but
> > we weren't applying it for 3D and cubemap textures.  I found a spec quote
> > in the narrative for GetTexImage that indicates we need to do it for those
> > as well.
> >
> > What do you think of this version?  I preserved your authorship as I wanted
> > you to get the credit for this bugfix - I just typed it up so that I could
> > make sure it actually fixed the test, and since I had it typed up, I figured
> > I'd send it out to save you some time...
> >
> 
> Hi Kenneth,
> 
> Great, this is the correct solution. I tried to make my original patch 
> work with _mesa_image_address3d() for all targets, as yours, but I 
> missed resetting SkipPixels and SkipRows for TEXTURE_3D, so it was 
> regressing some subcases.
> 
> Patch is:
> 
> Reviewed-by: Eduardo Lima Mitev 
> 
> I don't mind you taking authorship. Knowing the correct solution is way 
> more useful to me :). If you want, take the authorship, put my R-b and 
> push; otherwise let me know and I'll put your R-b and push it myself.
> 
> Thanks!
> 
> Eduardo

Reviewed-by: Kenneth Graunke 

Go ahead and push it :) Thanks!


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

Re: [Mesa-dev] [PATCH v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

2016-11-15 Thread Eduardo Lima Mitev 

On 11/15/2016 12:25 AM, Kenneth Graunke wrote:

From: Eduardo Lima Mitev 

This option was being ignored when packing compressed 3D and cube textures.

Fixes CTS test (on gen8+):
* GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore

v2: Drop API checks.
v3 (Ken): Just apply the existing code in more cases.
---
 src/mesa/drivers/common/meta.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

Hey Eduardo,

It looks like the existing code already tries to handle SkipImages - but
we weren't applying it for 3D and cubemap textures.  I found a spec quote
in the narrative for GetTexImage that indicates we need to do it for those
as well.

What do you think of this version?  I preserved your authorship as I wanted
you to get the credit for this bugfix - I just typed it up so that I could
make sure it actually fixed the test, and since I had it typed up, I figured
I'd send it out to save you some time...



Hi Kenneth,

Great, this is the correct solution. I tried to make my original patch 
work with _mesa_image_address3d() for all targets, as yours, but I 
missed resetting SkipPixels and SkipRows for TEXTURE_3D, so it was 
regressing some subcases.


Patch is:

Reviewed-by: Eduardo Lima Mitev 

I don't mind you taking authorship. Knowing the correct solution is way 
more useful to me :). If you want, take the authorship, put my R-b and 
push; otherwise let me know and I'll put your R-b and push it myself.


Thanks!

Eduardo


 --Ken

diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
index 5ab1e6c..99c85cf 100644
--- a/src/mesa/drivers/common/meta.c
+++ b/src/mesa/drivers/common/meta.c
@@ -3243,8 +3243,20 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,

   for (slice = 0; slice < depth; slice++) {
  void *dst;
- if (texImage->TexObject->Target == GL_TEXTURE_2D_ARRAY
- || texImage->TexObject->Target == GL_TEXTURE_CUBE_MAP_ARRAY) {
+ /* Section 8.11.4 (Texture Image Queries) of the GL 4.5 spec says:
+  *
+  *"For three-dimensional, two-dimensional array, cube map array,
+  * and cube map textures pixel storage operations are applied as
+  * if the image were two-dimensional, except that the additional
+  * pixel storage state values PACK_IMAGE_HEIGHT and
+  * PACK_SKIP_IMAGES are applied. The correspondence of texels to
+  * memory locations is as defined for TexImage3D in section 8.5."
+  */
+ switch (texImage->TexObject->Target) {
+ case GL_TEXTURE_3D:
+ case GL_TEXTURE_2D_ARRAY:
+ case GL_TEXTURE_CUBE_MAP:
+ case GL_TEXTURE_CUBE_MAP_ARRAY: {
 /* Setup pixel packing.  SkipPixels and SkipRows will be applied
  * in the decompress_texture_image() function's call to
  * glReadPixels but we need to compute the dest slice's address
@@ -3255,9 +3267,11 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
 packing.SkipRows = 0;
 dst = _mesa_image_address3d(, pixels, width, height,
 format, type, slice, 0, 0);
+break;
  }
- else {
+ default:
 dst = pixels;
+break;
  }
  result = decompress_texture_image(ctx, texImage, slice,
xoffset, yoffset, width, height,



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

[Mesa-dev] [PATCH v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

2016-11-14 Thread Kenneth Graunke 
From: Eduardo Lima Mitev 

This option was being ignored when packing compressed 3D and cube textures.

Fixes CTS test (on gen8+):
* GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore

v2: Drop API checks.
v3 (Ken): Just apply the existing code in more cases.
---
 src/mesa/drivers/common/meta.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

Hey Eduardo,

It looks like the existing code already tries to handle SkipImages - but
we weren't applying it for 3D and cubemap textures.  I found a spec quote
in the narrative for GetTexImage that indicates we need to do it for those
as well.

What do you think of this version?  I preserved your authorship as I wanted
you to get the credit for this bugfix - I just typed it up so that I could
make sure it actually fixed the test, and since I had it typed up, I figured
I'd send it out to save you some time...

 --Ken

diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
index 5ab1e6c..99c85cf 100644
--- a/src/mesa/drivers/common/meta.c
+++ b/src/mesa/drivers/common/meta.c
@@ -3243,8 +3243,20 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
 
   for (slice = 0; slice < depth; slice++) {
  void *dst;
- if (texImage->TexObject->Target == GL_TEXTURE_2D_ARRAY
- || texImage->TexObject->Target == GL_TEXTURE_CUBE_MAP_ARRAY) {
+ /* Section 8.11.4 (Texture Image Queries) of the GL 4.5 spec says:
+  *
+  *"For three-dimensional, two-dimensional array, cube map array,
+  * and cube map textures pixel storage operations are applied as
+  * if the image were two-dimensional, except that the additional
+  * pixel storage state values PACK_IMAGE_HEIGHT and
+  * PACK_SKIP_IMAGES are applied. The correspondence of texels to
+  * memory locations is as defined for TexImage3D in section 8.5."
+  */
+ switch (texImage->TexObject->Target) {
+ case GL_TEXTURE_3D:
+ case GL_TEXTURE_2D_ARRAY:
+ case GL_TEXTURE_CUBE_MAP:
+ case GL_TEXTURE_CUBE_MAP_ARRAY: {
 /* Setup pixel packing.  SkipPixels and SkipRows will be applied
  * in the decompress_texture_image() function's call to
  * glReadPixels but we need to compute the dest slice's address
@@ -3255,9 +3267,11 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
 packing.SkipRows = 0;
 dst = _mesa_image_address3d(, pixels, width, height,
 format, type, slice, 0, 0);
+break;
  }
- else {
+ default:
 dst = pixels;
+break;
  }
  result = decompress_texture_image(ctx, texImage, slice,
xoffset, yoffset, width, height,
-- 
2.10.2

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