[Mesa-dev] [PATCH 05/11] mesa: implement _mesa_GetTextureSubImage()
Laura Ekstrand
laura at jlekstrand.net
Wed Dec 17 16:03:27 PST 2014
On Sat, Dec 13, 2014 at 6:42 AM, Brian Paul <brianp at vmware.com> wrote:
>
> 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)
>
> 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).
> /**
> - * 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);
> }
>
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.
> +
> +
> +
> +/**
> + * 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;
> + }
> +
>
Why didn't you put xoffset %d + width %d > image_width %u ?
> + 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);
>
+ if (bw > 1 || bh > 1) {
> + /* offset must be multiple of block size */
>
This should be an INVALID_VALUE error according to the GL 4.5 spec.
> + 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) {
>
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.
> + if (yoffset % bw != 0) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetTextureSubImage(yoffset = %d)", yoffset);
> + return true;
> + }
> + }
>
Why is there no similar zoffset check? Is it because Mesa doesn't have a
concept of block depth?
> +
> + /* 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.
> + */
>
Note: xoffset + width != (GLint) texImage->Width is more permissive than
the spec document, which says (xoffset != 0) || (width != (GLint)
texImage->Width).
Again, the spec document says INVALID_VALUE.
> + if ((width % bw != 0) &&
> + (xoffset + width != (GLint) texImage->Width)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetTextureSubImage(width = %d)", width);
> + return true;
> + }
> +
>
Again, the spec document says INVALID_VALUE.
> + if ((height % bh != 0) &&
> + (yoffset + height != (GLint) texImage->Height)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetTextureSubImage(height = %d)", height);
> + return true;
> + }
> + }
> + }
>
No depth check?
> +
> + 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) {
>
I think this error might be a mistake in the spec. According to
https://www.opengl.org/registry/specs/ARB/get_texture_sub_image.txt, 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).
> + _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;
> + }
> +
>
You forgot to call legal_getteximage_target.
> + target = texObj->Target;
>
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).
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.
+ 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
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141217/c9fc2132/attachment-0001.html>
More information about the mesa-dev
mailing list