[Mesa-dev] [PATCH 3/3] getteximage: assume texture image is empty for non defined levels

[Juan A. Suarez Romero]

On Thu, 2018-04-12 at 09:35 +0200, Iago Toral wrote:
> On Wed, 2018-04-11 at 19:51 +0200, Juan A. Suarez Romero wrote:
> > Current code is returning an INVALID_OPERATION when trying to use
> > getTextureImage() on a level that has not been explicitly defined.
> > 
> > That is, we define a mipmapped Texture2D with 3 levels, and try to
> > use
> > GetTextureImage() for the 4th levels, and INVALID_OPERATION is
> > returned.
> > 
> > Nevertheless, such case is not listed as an error in OpenGL 4.6 spec,
> > section 8.11.4 ("Texture Image Queries"), where all the case errors
> > for
> > this function are defined. So it seems this is a valid operation.
> 
> It says this:
> 
> An INVALID_VALUE error is generated if level is negative or larger than
> the maximum allowable level.
> 

The maximum allowable level is not the number of levels defined for the texture,
but rather a value that depends on the texture target. This maximum allowable
level is defined in section 8.11.3; specifically:

"level determines which level-of-detail’s state is returned. The maximum value
of level depends on the texture target:

- For targets TEXTURE_CUBE_MAP and TEXTURE_CUBE_MAP_ARRAY, the maximum value is
  log2 of the value of MAX_CUBE_MAP_TEXTURE_SIZE

- For target TEXTURE_3D , the maximum value is log2 of the value of
  MAX_3D_TEXTURE_SIZE

- For targets TEXTURE_BUFFER, TEXTURE_RECTANGLE, TEXTURE_2D_MULTISAMPLE, and
  TEXTURE_2D_MULTISAMPLE_ARRAY,  which do not support mipmaps, the maximum
  value is zero

- For  all  other  texture targets  supported  by GetTex*LevelParameter*, the
  maximum value is log2 of the value of MAX_TEXTURE_SIZE"

So this maximum allowable level defines how many levels the texture can have,
rather the number of levels it has.

> > On the other hand, in section 8.22 ("Texture State and Proxy State")
> > it
> > states:
> > 
> >   "Each initial texture image is null. It has zero width, height, and
> >    depth, internal format RGBA, or R8 for buffer textures, component
> >    sizes set to zero and component types set to NONE, the compressed
> >    flag set to FALSE, a zero compressed size, and the bound buffer
> >    object name is zero."
> > 
> > We can assume that we are reading this initialized empty image when
> > calling GetTextureImage() with a non defined level.
> 
> Based on the above, I think we should just change the error code to be
> INVALID_VALUE instead of INVALID_OPERATION.
> 

Yes, we could inmediately return with an INVALID_VALUE error. I choosed to not
do it to make it explicitly that the error is not because there is no image for
the level, but because it reaches the other error cases defined for the
function.

"An INVALID_VALUE error is generated if xoffset + width is greater than
the  texture’s  width, yoffset + height
is  greater  than  the  texture’s  height, or zoffset + depth is greater than
the texture’s depth."


As per private discussion, I'll send a new version of this patch to just return
inmediately the INVALID_VALUE error if the image is empty, adding a comment why
we return this error.


	J.A.


> > With this assumption, we will reach one of the other error cases
> > defined
> > for the functions. At the end, this means that we will return a
> > different error to the caller.
> > 
> > This fixes arb_get_texture_sub_image piglit tests.
> > 
> > Signed-off-by: Juan A. Suarez Romero <jasuarez at igalia.com>
> > ---
> >  src/mesa/main/texgetimage.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/mesa/main/texgetimage.c
> > b/src/mesa/main/texgetimage.c
> > index 85d0ffd4770..0e4f030cb08 100644
> > --- a/src/mesa/main/texgetimage.c
> > +++ b/src/mesa/main/texgetimage.c
> > @@ -913,6 +913,7 @@ dimensions_error_check(struct gl_context *ctx,
> >                         const char *caller)
> >  {
> >     const struct gl_texture_image *texImage;
> > +   GLuint image_width, image_height, image_depth;
> >     int i;
> >  
> >     if (xoffset < 0) {
> > @@ -1004,37 +1005,39 @@ dimensions_error_check(struct gl_context
> > *ctx,
> >  
> >     texImage = select_tex_image(texObj, target, level, zoffset);
> >     if (!texImage) {
> > -      /* missing texture image */
> > -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(missing image)",
> > caller);
> > -      return true;
> > +      /* missing texture image; continue as initialized as empty */
> > +      _mesa_warning(ctx, "%s(missing image)", caller);
> >     }
> >  
> > -   if (xoffset + width > texImage->Width) {
> > +   image_width = texImage ? texImage->Width : 0;
> > +   if (xoffset + width > image_width) {
> >        _mesa_error(ctx, GL_INVALID_VALUE,
> >                    "%s(xoffset %d + width %d > %u)",
> > -                  caller, xoffset, width, texImage->Width);
> > +                  caller, xoffset, width, image_width);
> >        return true;
> >     }
> >  
> > -   if (yoffset + height > texImage->Height) {
> > +   image_height = texImage ? texImage->Height : 0;
> > +   if (yoffset + height > image_height) {
> >        _mesa_error(ctx, GL_INVALID_VALUE,
> >                    "%s(yoffset %d + height %d > %u)",
> > -                  caller, yoffset, height, texImage->Height);
> > +                  caller, yoffset, height, image_height);
> >        return true;
> >     }
> >  
> >     if (target != GL_TEXTURE_CUBE_MAP) {
> >        /* Cube map error checking was done above */
> > -      if (zoffset + depth > texImage->Depth) {
> > +      image_depth = texImage ? texImage->Depth : 0;
> > +      if (zoffset + depth > image_depth) {
> >           _mesa_error(ctx, GL_INVALID_VALUE,
> >                       "%s(zoffset %d + depth %d > %u)",
> > -                     caller, zoffset, depth, texImage->Depth);
> > +                     caller, zoffset, depth, image_depth);
> >           return true;
> >        }
> >     }
> >  
> >     /* Extra checks for compressed textures */
> > -   {
> > +   if (texImage) {
> >        GLuint bw, bh, bd;
> >        _mesa_get_format_block_size_3d(texImage->TexFormat, &bw, &bh,
> > &bd);
> >        if (bw > 1 || bh > 1 || bd > 1) {
> 
>