[Mesa-dev] [PATCH 05/11] mesa: implement _mesa_GetTextureSubImage()
Brian Paul
brianp at vmware.com
Thu Dec 18 07:32:21 PST 2014
On 12/17/2014 05:03 PM, Laura Ekstrand wrote:
>
>
> On Sat, Dec 13, 2014 at 6:42 AM, Brian Paul <brianp at vmware.com
> <mailto: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 ?
IMHO, the error message has enough info to convey the problem. If you
feel strongly that this needs to be fixed, there's a bunch of similar
instances (with more terse messages) in teximage.c that should be fixed
as well.
>
> + 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.
Fixed in v2.
>
> + 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.
Fixed in v2.
> Also, shouldn't it be yoffset % bh != 0 ? You have bw.
Fixed in v2.
>
> + 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?
Correct.
>
> +
> + /* 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).
I believe there was discussion about this a few years ago and Mesa's
behavior matches nvidia's driver. The same logic is used in the
glTexImage code. It allows sub-regions of compressed textures to be
read/written on block boundaries.
> Again, the spec document says INVALID_VALUE.
Fixed.
>
> + 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.
Fixed.
>
> + if ((height % bh != 0) &&
> + (yoffset + height != (GLint) texImage->Height)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetTextureSubImage(height = %d)", height);
> + return true;
> + }
> + }
> + }
>
> No depth check?
No. We have no 3D compression formats.
>
> +
> + 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
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.opengl.org_registry_specs_ARB_get-5Ftexture-5Fsub-5Fimage.txt&d=AwMFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=WWWcDPCoYwfOyILpH0kA7-k-qpSPTsIs1qycAmB5GNA&s=ZhWz0mRK6xg9KPHqEvdXdXdSYxLyX81SGDpO0VoV-nw&e=>,
> 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).
Actually, a later patch in the series changed this to INVALID_OPERATION
(and fixed the error string). I'll move that bit into this patch.
INVALID_OPERATION seems to be the right thing.
>
> + _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.
I don't think that's needed since it's not possible for texObj->Target
to be an invalid value at this point.
>
> + 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).
Ugh, something got lost from my early versions of the
GL_ARB_get_texture_sub_image spec to what's in the final spec, and the
4.5 spec.
The problem is OpenGL doesn't disallow specifying different dimensions
for each of the cube map faces. For example, it's perfectly legal to
create a cube map texture like this:
glTexImage2D(POSITIVE_X, width=32, height=32)
glTexImage2D(NEGATIVE_X, width=16, height=16)
glTexImage2D(POSITIVE_Y, width=64, height=64)
etc.
You can even have different internal formats per face!
The texture will be flagged as 'incomplete' and you can't render with
it. But you can create such a texture and you can query the face
dimensions with glGetTexLevelParameter() and get back the per-face
sizes. Plus, glGetTexImage operates per-face. (I should probably whip
up a piglit test to verify this).
So, what should happen with glGetTextureSubImage() if we try to get two
or more mismatched-size faces at once? I can't find any spec language
about this. Maybe just raise GL_INVALID_OPERATION??
My first few drafts of the GL_ARB_get_texture_sub_image spec had a
target parameter which only allowed
GL_TEXTURE_CUBE_MAP_POSITIVE/NEGATIVE_X/Y/Z so that this situation would
be handled consistently.
I didn't see that this got changed in the spec so I implemented what I
originally intended (only allow getting one face/slice at a time).
BTW, it would be nice in Mesa if we could treat GL_TEXTURE_CUBE_MAP
textures more like GL_TEXTURE_CUBE_MAP_ARRAY textures. That is, the
faces would just be treated as slices in a depth=6 array.
Ideally, gl_texture_object::Image[face][level] would become
gl_texture_object::Image[level]. But we can't really do that since
according to the spec, we need to keep track of possibly different
dimensions and formats for each face.
This is a mess. What do you think about the GL_INVALID_OPERATION idea?
In any case, I'm on vacation after Friday so I probably won't have time
to go further on this until the new year.
-Brian
> 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 <mailto:mesa-dev at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=AwMFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=WWWcDPCoYwfOyILpH0kA7-k-qpSPTsIs1qycAmB5GNA&s=GrUeebJuM5x75DOASs1ezQdYEv5z61wojf4LHxr7Rvc&e=>
>
More information about the mesa-dev
mailing list