Re: [Mesa-dev] [PATCH 3/3] getteximage: assume texture image is empty for non defined levels
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 > > --- > > 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 (yoff
Re: [Mesa-dev] [PATCH 3/3] getteximage: assume texture image is empty for non defined levels
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. > 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. > 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 > --- > 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) { ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/3] getteximage: assume texture image is empty for non defined levels
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. 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. 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 --- 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) { -- 2.14.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev