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

[Fredrik Höglund]

On Wednesday 10 May 2017, Nicolai Hähnle wrote:
> 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?

Because it makes no difference in practice. The target parameter is only
used to check that the texture is not an array texture before adding the
border width to the offsets. The border width is also always zero in core
contexts, and we don't expose the DSA extension in compatibility contexts.

But I noticed something else below:

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

GL_TEXTURE_CUBE_MAP is not a legal target in glTexImage*D.

> 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) {

This loop should take the zoffset and depth into account, as in
texturesubimage, and zoffset should not be passed as a parameter to
compressed_texture_sub_image.

> >           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;
> >        }
> >     }
> >
> 
> 
>