<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Sat, Dec 13, 2014 at 6:42 AM, Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">One of the two new functions in GL_ARB_get_texture_sub_image. --- src/mesa/main/texgetimage.c | 305 ++++++++++++++++++++++++++++++++++++++------ src/mesa/main/texgetimage.h | 8 ++ 2 files changed, 277 insertions(+), 36 deletions(-) diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c index 71c25bb..e1f238b 100644 --- a/src/mesa/main/texgetimage.c +++ b/src/mesa/main/texgetimage.c @@ -44,6 +44,7 @@ #include "texcompress.h" #include "texgetimage.h" #include "teximage.h" +#include "texobj.h" #include "texstore.h" @@ -803,41 +804,36 @@ legal_getteximage_target(struct gl_context *ctx, GLenum target) </blockquote><div>In my work with DSA textures, I have discovered that it's more robust to pass in both the texture object and target. I originally just passed in the texObj instead of the target in error checks and used texObj->Target, but I failed a number of Piglit tests afterwards. This is because, for non-DSA texture objects, the target passed into the function and the texObj->Target don't always match (especially for proxy targets). </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /** - * Do error checking for a glGetTexImage() call. + * Do error checking for a glGetTexImage() or glGetTextureSubImage() call. * \return GL_TRUE if any error, GL_FALSE if no errors. */ static GLboolean -getteximage_error_check(struct gl_context *ctx, GLenum target, GLint level, +getteximage_error_check(struct gl_context *ctx, + struct gl_texture_object *texObj, GLint level, GLenum format, GLenum type, GLsizei clientMemSize, - GLvoid *pixels ) + GLvoid *pixels, const char *caller) { - struct gl_texture_object *texObj; + GLenum target = texObj->Target; struct gl_texture_image *texImage; const GLint maxLevels = _mesa_max_texture_levels(ctx, target); - const GLuint dimensions = (target == GL_TEXTURE_3D) ? 3 : 2; GLenum baseFormat, err; - if (!legal_getteximage_target(ctx, target)) { - _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexImage(target=0x%x)", target); - return GL_TRUE; - } - assert(maxLevels != 0); if (level < 0 || level >= maxLevels) { - _mesa_error( ctx, GL_INVALID_VALUE, "glGetTexImage(level)" ); + _mesa_error(ctx, GL_INVALID_VALUE, "%s(level)", caller); return GL_TRUE; } err = _mesa_error_check_format_and_type(ctx, format, type); if (err != GL_NO_ERROR) { - _mesa_error(ctx, err, "glGetTexImage(format/type)"); + _mesa_error(ctx, err, "%s(format/type)", caller); return GL_TRUE; } texObj = _mesa_get_current_tex_object(ctx, target); if (!texObj) { - _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexImage(target)"); + _mesa_error(ctx, GL_INVALID_ENUM, "%s(target)", caller); return GL_TRUE; } @@ -854,64 +850,79 @@ getteximage_error_check(struct gl_context *ctx, GLenum target, GLint level, */ if (_mesa_is_color_format(format) && !_mesa_is_color_format(baseFormat)) { - _mesa_error(ctx, GL_INVALID_OPERATION, "glGetTexImage(format mismatch)"); + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(format mismatch)", caller); return GL_TRUE; } else if (_mesa_is_depth_format(format) && !_mesa_is_depth_format(baseFormat) && !_mesa_is_depthstencil_format(baseFormat)) { - _mesa_error(ctx, GL_INVALID_OPERATION, "glGetTexImage(format mismatch)"); + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(format mismatch)", caller); return GL_TRUE; } else if (_mesa_is_stencil_format(format) && !ctx->Extensions.ARB_texture_stencil8) { - _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexImage(format=GL_STENCIL_INDEX)"); + _mesa_error(ctx, GL_INVALID_ENUM, "%s(format=GL_STENCIL_INDEX)", caller); return GL_TRUE; } else if (_mesa_is_ycbcr_format(format) && !_mesa_is_ycbcr_format(baseFormat)) { - _mesa_error(ctx, GL_INVALID_OPERATION, "glGetTexImage(format mismatch)"); + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(format mismatch)", caller); return GL_TRUE; } else if (_mesa_is_depthstencil_format(format) && !_mesa_is_depthstencil_format(baseFormat)) { - _mesa_error(ctx, GL_INVALID_OPERATION, "glGetTexImage(format mismatch)"); + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(format mismatch)", caller); return GL_TRUE; } else if (_mesa_is_enum_format_integer(format) != _mesa_is_format_integer(texImage->TexFormat)) { - _mesa_error(ctx, GL_INVALID_OPERATION, "glGetTexImage(format mismatch)"); + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(format mismatch)", caller); return GL_TRUE; } - if (!_mesa_validate_pbo_access(dimensions, &ctx->Pack, texImage->Width, - texImage->Height, texImage->Depth, - format, type, clientMemSize, pixels)) { + return GL_FALSE; +} + + +/** + * Do error checking related to the PBO and image size. + */ +static bool +pbo_error_check(struct gl_context *ctx, GLenum target, + GLenum format, GLenum type, + GLsizei width, GLsizei height, GLsizei depth, + GLsizei bufferSize, void *pixels, + const char *caller) +{ + const GLuint dimensions = (target == GL_TEXTURE_3D) ? 3 : 2; + + if (!_mesa_validate_pbo_access(dimensions, &ctx->Pack, + width, height, depth, + format, type, bufferSize, pixels)) { if (_mesa_is_bufferobj(ctx->Pack.BufferObj)) { _mesa_error(ctx, GL_INVALID_OPERATION, - "glGetTexImage(out of bounds PBO access)"); + "%s(out of bounds PBO access)", caller); } else { _mesa_error(ctx, GL_INVALID_OPERATION, - "glGetnTexImageARB(out of bounds access:" - " bufSize (%d) is too small)", clientMemSize); + "%s(out of bounds access: bufSize (%d) is too small)", + caller, bufferSize); } - return GL_TRUE; + return true; } if (_mesa_is_bufferobj(ctx->Pack.BufferObj)) { /* PBO should not be mapped */ if (_mesa_check_disallowed_mapping(ctx->Pack.BufferObj)) { _mesa_error(ctx, GL_INVALID_OPERATION, - "glGetTexImage(PBO is mapped)"); - return GL_TRUE; + "%s(PBO is mapped)", caller); + return true; } } - return GL_FALSE; + return false; } - /** * Get texture image. Called by glGetTexImage. * @@ -932,21 +943,36 @@ _mesa_GetnTexImageARB( GLenum target, GLint level, GLenum format, FLUSH_VERTICES(ctx, 0); - if (getteximage_error_check(ctx, target, level, format, type, - bufSize, pixels)) { + if (!legal_getteximage_target(ctx, target)) { + _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexImage(target=0x%x)", target); return; } - if (!_mesa_is_bufferobj(ctx->Pack.BufferObj) && !pixels) { - /* not an error, do nothing */ + texObj = _mesa_get_current_tex_object(ctx, target); + + if (getteximage_error_check(ctx, texObj, level, format, type, + bufSize, pixels, "glGet[n]TexImage")) { return; } - texObj = _mesa_get_current_tex_object(ctx, target); texImage = _mesa_select_tex_image(ctx, texObj, target, level); + if (!texImage) { + return; /* no texture image */ + } - if (_mesa_is_zero_size_texture(texImage)) + if (pbo_error_check(ctx, target, format, type, + texImage->Width, texImage->Height, texImage->Depth, + bufSize, pixels, "glGet[n]TexImage")) { return; + } + + if (_mesa_is_zero_size_texture(texImage)) { + return; /* nothing to get */ + } + + if (!_mesa_is_bufferobj(ctx->Pack.BufferObj) && !pixels) { + return; /* not an error, do nothing */ + } if (MESA_VERBOSE & (VERBOSE_API | VERBOSE_TEXTURE)) { _mesa_debug(ctx, "glGetTexImage(tex %u) format = %s, w=%d, h=%d," @@ -1110,3 +1136,210 @@ _mesa_GetCompressedTexImage(GLenum target, GLint level, GLvoid *img) { _mesa_GetnCompressedTexImageARB(target, level, INT_MAX, img); } </blockquote><div> </div><div>Note: When this gets merged with my DSA work, the target should maybe get passed into this function. This is because DSA and traditional functions share a backend function, and that backend function will have to resemble your GetTextureSubImage function (thus calling dimensions_error_check). As aforementioned, not passing the target could cause trouble. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + + +/** + * Error-check the offset and size arguments to + * glGet[Compressed]TextureSubImage(). + * \return true if error, false if no error. + */ +static bool +dimensions_error_check(struct gl_context *ctx, + struct gl_texture_image *texImage, + GLint xoffset, GLint yoffset, GLint zoffset, + GLsizei width, GLsizei height, GLsizei depth) +{ + const GLenum target = texImage->TexObject->Target; + + switch (target) { + case GL_TEXTURE_1D: + if (yoffset != 0) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(1D, yoffset = %d)\n", yoffset); + return true; + } + if (height != 1) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(1D, height = %d)\n", height); + return true; + } + /* fall-through */ + case GL_TEXTURE_1D_ARRAY: + case GL_TEXTURE_2D: + case GL_TEXTURE_RECTANGLE: + if (zoffset != 0) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(zoffset = %d)\n", zoffset); + return true; + } + if (depth != 1) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(depth = %d)\n", depth); + return true; + } + break; + default: + ; /* nothing */ + } + + if (xoffset < 0) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(xoffset = %d)\n", xoffset); + return true; + } + + if (yoffset < 0) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(yoffset = %d)\n", yoffset); + return true; + } + + if (zoffset < 0) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(zoffset = %d)\n", zoffset); + return true; + } + </blockquote><div>Why didn't you put xoffset %d + width %d > image_width %u ? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (xoffset + width > texImage->Width) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(xoffset %d + width %d > %u)\n", + xoffset, width, texImage->Width); + return true; + } + + if (yoffset + height > texImage->Height) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(yoffset %d + height %d > %u)\n", + yoffset, height, texImage->Height); + return true; + } + + if (zoffset + depth > texImage->Depth) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(zoffset %d + depth %d > %u)\n", + zoffset, depth, texImage->Depth); + return true; + } + + /* Extra checks for compressed textures */ + { + GLuint bw, bh; + _mesa_get_format_block_size(texImage->TexFormat, &bw, &bh); </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (bw > 1 || bh > 1) { + /* offset must be multiple of block size */ </blockquote><div>This should be an INVALID_VALUE error according to the GL 4.5 spec. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (xoffset % bw != 0) { + _mesa_error(ctx, GL_INVALID_OPERATION, + "glGetTextureSubImage(xoffset = %d)", xoffset); + return true; + } + if (target != GL_TEXTURE_1D && target != GL_TEXTURE_1D_ARRAY) { </blockquote><div>This should be an INVALID_VALUE error according to the GL 4.5 spec. Also, shouldn't it be yoffset % bh != 0 ? You have bw. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (yoffset % bw != 0) { + _mesa_error(ctx, GL_INVALID_OPERATION, + "glGetTextureSubImage(yoffset = %d)", yoffset); + return true; + } + } </blockquote><div>Why is there no similar zoffset check? Is it because Mesa doesn't have a concept of block depth? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* The size must be a multiple of bw x bh, or we must be using a + * offset+size that exactly hits the edge of the image. + */ </blockquote><div>Note: xoffset + width != (GLint) texImage->Width is more permissive than the spec document, which says (xoffset != 0) || (width != (GLint) texImage->Width). </div><div>Again, the spec document says INVALID_VALUE. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if ((width % bw != 0) && + (xoffset + width != (GLint) texImage->Width)) { + _mesa_error(ctx, GL_INVALID_OPERATION, + "glGetTextureSubImage(width = %d)", width); + return true; + } + </blockquote><div>Again, the spec document says INVALID_VALUE. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if ((height % bh != 0) && + (yoffset + height != (GLint) texImage->Height)) { + _mesa_error(ctx, GL_INVALID_OPERATION, + "glGetTextureSubImage(height = %d)", height); + return true; + } + } + } </blockquote><div>No depth check? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + if (width == 0 || height == 0 || depth == 0) { + /* Not an error, but nothing to do. Return 'true' so that the + * caller simply returns. + */ + return true; + } + + return false; +} + + + +void GLAPIENTRY +_mesa_GetTextureSubImage(GLuint texture, GLint level, + GLint xoffset, GLint yoffset, GLint zoffset, + GLsizei width, GLsizei height, GLsizei depth, + GLenum format, GLenum type, GLsizei bufSize, + void *pixels) +{ + struct gl_texture_object *texObj; + struct gl_texture_image *texImage; + GLenum target; + + GET_CURRENT_CONTEXT(ctx); + + texObj = _mesa_lookup_texture(ctx, texture); + if (!texObj) { </blockquote><div> </div><div>I think this error might be a mistake in the spec. According to <a href="https://www.opengl.org/registry/specs/ARB/get_texture_sub_image.txt" target="_blank">https://www.opengl.org/registry/specs/ARB/get_texture_sub_image.txt</a>, it should be INVALID_OPERATION in the case where the user passed in an unknown texture name. Oddly enough, the OpenGL 4.5 core spec (30.10.2014) has INVALID_VALUE (consistent with what you have here) in section 8.11 Texture Queries. But that is very strange because all the DSA functions consistently throw INVALID_OPERATION for this error (such as GetTextureImage and TextureSubImage*D, etc.) in the OpenGL 4.5 spec. I'm not sure why they wouldn't agree. In my DSA patch series, I have a convenience function for throwing INVALID_OPERATION for an unknown texture name because this occurs absolutely everywhere (_mesa_lookup_texture_err). </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(invalid texture ID %u\n", texture); + return; + } + + /* common error checking */ + if (getteximage_error_check(ctx, texObj, level, format, type, bufSize, + pixels, "glGetTextureSubImage")) { + return; + } + </blockquote><div> </div><div>You forgot to call legal_getteximage_target. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + target = texObj->Target; </blockquote><div> </div><div>This doesn't seem like it will return more than one face of the cube map. In fact, it will throw INVALID_OPERATION if the user tries to do so (depth != 1). But the GL 4.5 spec says the user should be able to get as many faces as he or she wants from GetTextureSubImage (up to all six). </div><div>In my GetTextureImage patch, I just call the backend _mesa_get_texture_image multiple times. When they are merged, the backend will be _mesa_get_texture_sub_image and GetTextureSubImage can call that multiple times in the same way. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (target == GL_TEXTURE_CUBE_MAP) { + /* convert z to face/target */ + if (zoffset >= 6) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(cube, zoffset = %d\n", zoffset); + return; + } + if (depth != 1) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glGetTextureSubImage(cube, depth = %d\n", depth); + return; + } + target = GL_TEXTURE_CUBE_MAP_POSITIVE_X + zoffset; + } + + texImage = _mesa_select_tex_image(ctx, texObj, target, level); + + /* check dimensions */ + if (dimensions_error_check(ctx, texImage, xoffset, yoffset, zoffset, + width, height, depth)) { + return; + } + + if (pbo_error_check(ctx, target, format, type, + width, height, depth, + bufSize, pixels, "glGetTextureSubImage")) { + return; + } + + if (_mesa_is_zero_size_texture(texImage)) { + return; /* nothing to get */ + } + + if (!_mesa_is_bufferobj(ctx->Pack.BufferObj) && pixels == NULL) { + return; /* not an error, do nothing */ + } + + _mesa_lock_texture(ctx, texObj); + { + ctx->Driver.GetTexSubImage(ctx, xoffset, yoffset, zoffset, width, height, + depth, format, type, pixels, texImage); + } + _mesa_unlock_texture(ctx, texObj); +} diff --git a/src/mesa/main/texgetimage.h b/src/mesa/main/texgetimage.h index c9af5b9..40f152c 100644 --- a/src/mesa/main/texgetimage.h +++ b/src/mesa/main/texgetimage.h @@ -65,4 +65,12 @@ extern void GLAPIENTRY _mesa_GetnCompressedTexImageARB(GLenum target, GLint level, GLsizei bufSize, GLvoid *img); +extern void GLAPIENTRY +_mesa_GetTextureSubImage(GLuint texture, GLint level, + GLint xoffset, GLint yoffset, GLint zoffset, + GLsizei width, GLsizei height, GLsizei depth, + GLenum format, GLenum type, GLsizei bufSize, + void *pixels); + + #endif /* TEXGETIMAGE_H */ -- 1.9.1 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div></div>