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

[Eduardo Lima Mitev]

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.

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,
>