[Mesa-dev] [PATCH] texgetimage: consolidate 1D array handling code.

[Brian Paul]

Just two minor nits below.


On 11/10/2015 07:34 PM, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This should fix the getteximage-depth test that currently asserts.
>
> I was hitting problem with virgl as well in this area.
>
> This moves the 1D array handling code to a single place.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>   src/mesa/main/texgetimage.c | 24 +++++++++---------------
>   1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
> index 945890a..200d1ca 100644
> --- a/src/mesa/main/texgetimage.c
> +++ b/src/mesa/main/texgetimage.c
> @@ -88,12 +88,6 @@ get_tex_depth(struct gl_context *ctx, GLuint dimensions,
>         return;
>      }
>
> -   if (texImage->TexObject->Target == GL_TEXTURE_1D_ARRAY) {
> -      depth = height;
> -      height = 1;
> -   }
> -
> -   assert(zoffset + depth <= texImage->Depth);
>      for (img = 0; img < depth; img++) {
>         GLubyte *srcMap;
>         GLint srcRowStride;
> @@ -141,7 +135,6 @@ get_tex_depth_stencil(struct gl_context *ctx, GLuint dimensions,
>      assert(type == GL_UNSIGNED_INT_24_8 ||
>             type == GL_FLOAT_32_UNSIGNED_INT_24_8_REV);
>
> -   assert(zoffset + depth <= texImage->Depth);
>      for (img = 0; img < depth; img++) {
>         GLubyte *srcMap;
>         GLint rowstride;
> @@ -233,7 +226,6 @@ get_tex_ycbcr(struct gl_context *ctx, GLuint dimensions,
>   {
>      GLint img, row;
>
> -   assert(zoffset + depth <= texImage->Depth);
>      for (img = 0; img < depth; img++) {
>         GLubyte *srcMap;
>         GLint rowstride;
> @@ -431,13 +423,6 @@ get_tex_rgba_uncompressed(struct gl_context *ctx, GLuint dimensions,
>      bool needsRebase;
>      void *rgba = NULL;
>
> -   if (texImage->TexObject->Target == GL_TEXTURE_1D_ARRAY) {
> -      depth = height;
> -      height = 1;
> -      zoffset = yoffset;
> -      yoffset = 0;
> -   }
> -
>      /* Depending on the base format involved we may need to apply a rebase
>       * transform (for example: if we download to a Luminance format we want
>       * G=0 and B=0).
> @@ -737,6 +722,15 @@ _mesa_GetTexSubImage_sw(struct gl_context *ctx,
>         pixels = ADD_POINTERS(buf, pixels);
>      }
>

I'd probably put a comment on this conditional saying something like

/* for all array textures, the Z axis selects the layer */

> +   if (texImage->TexObject->Target == GL_TEXTURE_1D_ARRAY) {
> +      depth = height;
> +      height = 1;
> +      zoffset = yoffset;
> +      yoffset = 0;
> +      assert(zoffset + depth <= texImage->Height);
> +   } else
> +      assert(zoffset + depth <= texImage->Depth);

I like seeing braces on the else clause to match the if clause.

Reviewed-by: Brian Paul <brianp at vmware.com>


> +
>      if (get_tex_memcpy(ctx, xoffset, yoffset, zoffset, width, height, depth,
>                         format, type, pixels, texImage)) {
>         /* all done */
>