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

[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 <elima at igalia.com>
>>
>> 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 <elima at igalia.com>
>
> 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(&packing, 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev