[Mesa-dev] [PATCH] mesa: fix *TextureSubImage*D() for GL_TEXTURE_CUBE_MAP target

[Nicolai Hähnle]

On 10.05.2017 09:23, Timothy Arceri wrote:
> Noticed by inspection. The assert should have caught this already
> but seems to have been incorrectly removed in b8939fd3d1544f.

How on earth is there no piglit test that catches the texturesubimage 
thing? Do you think you can add one?

> ---
>
>  I'm not overly familiar with this code but this seems to be how
>  it's intended to be used.
>
>  src/mesa/main/teximage.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index 2486704..1dd53ac 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -2669,20 +2669,21 @@ _mesa_update_fbo_texture(struct gl_context *ctx,
>
>  /**
>   * If the texture object's GenerateMipmap flag is set and we've
>   * changed the texture base level image, regenerate the rest of the
>   * mipmap levels now.
>   */
>  static inline void
>  check_gen_mipmap(struct gl_context *ctx, GLenum target,
>                   struct gl_texture_object *texObj, GLint level)
>  {
> +   assert(target != GL_TEXTURE_CUBE_MAP);

I don't think this is correct. teximage calls this function 
unconditionally, regardless of texture target.

The other changes look fine, but I'm a bit suspicious as noted above...

Cheers,
Nicolai


>     if (texObj->GenerateMipmap &&
>         level == texObj->BaseLevel &&
>         level < texObj->MaxLevel) {
>        assert(ctx->Driver.GenerateMipmap);
>        ctx->Driver.GenerateMipmap(ctx, target, texObj);
>     }
>  }
>
>
>  /** Debug helper: override the user-requested internal format */
> @@ -3369,21 +3370,22 @@ texturesubimage(struct gl_context *ctx, GLuint dims,
>           return;
>        }
>
>        imageStride = _mesa_image_image_stride(&ctx->Unpack, width, height,
>                                               format, type);
>        /* Copy in each face. */
>        for (i = zoffset; i < zoffset + depth; ++i) {
>           texImage = texObj->Image[i][level];
>           assert(texImage);
>
> -         _mesa_texture_sub_image(ctx, 3, texObj, texImage, texObj->Target,
> +         _mesa_texture_sub_image(ctx, 3, texObj, texImage,
> +                                 GL_TEXTURE_CUBE_MAP_POSITIVE_X + i,
>                                   level, xoffset, yoffset, 0,
>                                   width, height, 1, format,
>                                   type, pixels, true);
>           pixels = (GLubyte *) pixels + imageStride;
>        }
>     }
>     else {
>        texImage = _mesa_select_tex_image(texObj, texObj->Target, level);
>        assert(texImage);
>
> @@ -4758,23 +4760,23 @@ _mesa_CompressedTextureSubImage3D(GLuint texture, GLint level, GLint xoffset,
>                       "glCompressedTextureSubImage3D(cube map incomplete)");
>           return;
>        }
>
>        /* Copy in each face. */
>        for (i = 0; i < 6; ++i) {
>           texImage = texObj->Image[i][level];
>           assert(texImage);
>
>           compressed_texture_sub_image(ctx, 3, texObj, texImage,
> -                                      texObj->Target, level, xoffset, yoffset,
> -                                      zoffset, width, height, 1, format,
> -                                      imageSize, pixels);
> +                                      GL_TEXTURE_CUBE_MAP_POSITIVE_X + i,
> +                                      level, xoffset, yoffset, zoffset, width,
> +                                      height, 1, format, imageSize, pixels);
>
>           /* Compressed images don't have a client format */
>           image_stride = _mesa_format_image_size(texImage->TexFormat,
>                                                  texImage->Width,
>                                                  texImage->Height, 1);
>
>           pixels += image_stride;
>           imageSize -= image_stride;
>        }
>     }
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.