[Mesa-dev] [PATCH 08/13] mesa: overhaul the glGetTexImage code
Brian Paul
brianp at vmware.com
Wed Jul 15 16:40:37 PDT 2015
On 07/14/2015 03:54 PM, Ilia Mirkin wrote:
> On Mon, Jul 13, 2015 at 9:21 PM, Brian Paul <brianp at vmware.com> wrote:
>> 1. Reorganize the error checking code.
>> 2. Lay groundwork for getting sub images.
>> 3. Implement _mesa_GetnTexImageARB(), _mesa_GetTexImage() and
>> _mesa_GetTextureImage() all in terms of get_texture_image().
>> ---
>> src/mesa/main/texgetimage.c | 539 +++++++++++++++++++++++++++++---------------
>> 1 file changed, 352 insertions(+), 187 deletions(-)
>>
>> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
>> index e180a4c..b74517a 100644
>> --- a/src/mesa/main/texgetimage.c
>> +++ b/src/mesa/main/texgetimage.c
>> @@ -49,6 +49,13 @@
>> #include "format_utils.h"
>> #include "pixeltransfer.h"
>>
>> +
>> +/** Some magic numbers used when getting whole texture image */
>> +#define WHOLE_WIDTH -123
>> +#define WHOLE_HEIGHT -123
>> +#define WHOLE_DEPTH -123
>
> IMHO this is incredibly dirty. There simplest way to avoid this is to
> move the gl_texture_image retrieval logic off into a helper and pass
> the gl_texture_image in directly into get_texture_image, along with
> optionally, the full image width/height/depth.
I reworked the code to do that, but it's a bit more code overall. It's
kind of tricky. To pass the texture dims to the error checking
function, you need to lookup the texture image, but that involves some
error checking too.
>
>> +
>> +
>> /**
>> * Can the given type represent negative values?
>> */
>> @@ -891,27 +898,59 @@ legal_getteximage_target(struct gl_context *ctx, GLenum target, bool dsa)
>>
>>
>> /**
>> - * Do error checking for a glGetTex(ture)Image() call.
>> - * \return GL_TRUE if any error, GL_FALSE if no errors.
>> + * Do error checking for all (non-compressed) get-texture-image functions.
>> + * \param target user-provided texture target, or 0 if called from DSA function
>> + * \return true if any error, false if no errors.
>> */
>> -static GLboolean
>> +static bool
>> getteximage_error_check(struct gl_context *ctx,
>> - struct gl_texture_image *texImage,
>> + struct gl_texture_object *texObj,
>> GLenum target, GLint level,
>> GLenum format, GLenum type, GLsizei clientMemSize,
>> - GLvoid *pixels, bool dsa)
>> + GLvoid *pixels, const char *caller)
>> {
>> - const GLint maxLevels = _mesa_max_texture_levels(ctx, target);
>> - const GLuint dimensions = (target == GL_TEXTURE_3D) ? 3 : 2;
>> - GLenum baseFormat;
>> - const char *suffix = dsa ? "ture" : "";
>> + struct gl_texture_image *texImage;
>> + GLenum baseFormat, err;
>> + GLint maxLevels;
>> + const bool dsa = (target == 0);
>
> So now I can pass a GL_NONE target into these glGetTex* functions and
> get something back? [Perhaps that's guarded elsehwere, but this diff
> is hard to grok. Perhaps there's a tree I can look at somewhere?]
Sorry, the code is in my local tree. I could send you the complete
texgetimage.c file if that would help.
>
>>
>> - assert(texImage);
>> - assert(maxLevels != 0);
>> + if (!texObj || texObj->Target == 0) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid texture)", caller);
>> + return true;
>> + }
>> +
>> + if (dsa) {
>> + target = texObj->Target;
>
> in the non-dsa case, can target ever != texObj->Target? i.e. can you
> just use this unconditionally?
I've changed this too.
>
>> + }
>> +
>> + if (!legal_getteximage_target(ctx, target, dsa)) {
>> + _mesa_error(ctx, GL_INVALID_ENUM, "%s(target=%s)", caller,
>> + _mesa_lookup_enum_by_nr(texObj->Target));
>> + return true;
>> + }
>> +
>> + err = _mesa_error_check_format_and_type(ctx, format, type);
>> + if (err != GL_NO_ERROR) {
>> + _mesa_error(ctx, err, "%s(format/type)", caller);
>> + return true;
>> + }
>> +
>> + maxLevels = _mesa_max_texture_levels(ctx, target);
>> if (level < 0 || level >= maxLevels) {
>> _mesa_error(ctx, GL_INVALID_VALUE,
>> - "glGetTex%sImage(level out of range)", suffix);
>> - return GL_TRUE;
>> + "%s(level = %d)", caller, level);
>> + return true;
>> + }
>> +
>> + if (target == GL_TEXTURE_CUBE_MAP) {
>> + target = GL_TEXTURE_CUBE_MAP_POSITIVE_X;
>> + }
>> +
>> + texImage = _mesa_select_tex_image(texObj, target, level);
>> + if (!texImage) {
>> + /* non-existant texture image */
>> + _mesa_error(ctx, GL_INVALID_OPERATION, "%s", caller);
>> + return true;
>> }
>>
>> /*
>> @@ -927,247 +966,325 @@ getteximage_error_check(struct gl_context *ctx,
>> if (_mesa_is_color_format(format)
>> && !_mesa_is_color_format(baseFormat)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glGetTex%sImage(format mismatch)", suffix);
>> - return GL_TRUE;
>> + "%s(format mismatch)", caller);
>> + return true;
>> }
>> else if (_mesa_is_depth_format(format)
>> && !_mesa_is_depth_format(baseFormat)
>> && !_mesa_is_depthstencil_format(baseFormat)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glGetTex%sImage(format mismatch)", suffix);
>> - return GL_TRUE;
>> + "%s(format mismatch)", caller);
>> + return true;
>> }
>> else if (_mesa_is_stencil_format(format)
>> && !ctx->Extensions.ARB_texture_stencil8) {
>> _mesa_error(ctx, GL_INVALID_ENUM,
>> - "glGetTex%sImage(format=GL_STENCIL_INDEX)", suffix);
>> - return GL_TRUE;
>> + "%s(format=GL_STENCIL_INDEX)", caller);
>> + return true;
>> }
>> else if (_mesa_is_ycbcr_format(format)
>> && !_mesa_is_ycbcr_format(baseFormat)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glGetTex%sImage(format mismatch)", suffix);
>> - return GL_TRUE;
>> + "%s(format mismatch)", caller);
>> + return true;
>> }
>> else if (_mesa_is_depthstencil_format(format)
>> && !_mesa_is_depthstencil_format(baseFormat)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glGetTex%sImage(format mismatch)", suffix);
>> - return GL_TRUE;
>> + "%s(format mismatch)", caller);
>> + return true;
>> }
>> else if (!_mesa_is_stencil_format(format) &&
>> _mesa_is_enum_format_integer(format) !=
>> _mesa_is_format_integer(texImage->TexFormat)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glGetTex%sImage(format mismatch)", suffix);
>> - return GL_TRUE;
>> + "%s(format mismatch)", caller);
>> + return true;
>> }
>>
>> - if (!_mesa_validate_pbo_access(dimensions, &ctx->Pack, texImage->Width,
>> - texImage->Height, texImage->Depth,
>> + return false;
>> +}
>> +
>> +
>> +/**
>> + * Do PBO-related error checking.
>> + * \return true if there was an error (or the GetTexImage is to be a no-op)
>> + */
>> +static bool
>> +pbo_error_check(struct gl_context *ctx, GLenum target,
>> + GLsizei width, GLsizei height, GLsizei depth,
>> + GLenum format, GLenum type, GLsizei clientMemSize,
>> + GLvoid *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, clientMemSize, pixels)) {
>> if (_mesa_is_bufferobj(ctx->Pack.BufferObj)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glGetTex%sImage(out of bounds PBO access)", suffix);
>> + "%s(out of bounds PBO access)", caller);
>> } else {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "%s(out of bounds access:"
>> - " bufSize (%d) is too small)",
>> - dsa ? "glGetTextureImage" : "glGetnTexImageARB",
>> - clientMemSize);
>> + "%s(out of bounds access: bufSize (%d) is too small)",
>> + caller, clientMemSize);
>> }
>> - 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,
>> - "glGetTex%sImage(PBO is mapped)", suffix);
>> - return GL_TRUE;
>> + "%s(PBO is mapped)", caller);
>> + return true;
>> }
>> }
>>
>> - return GL_FALSE;
>> + if (!_mesa_is_bufferobj(ctx->Pack.BufferObj) && !pixels) {
>> + /* not an error, do nothing */
>> + return true;
>> + }
>> +
>> + return false;
>> }
>>
>>
>> /**
>> - * This is the implementation for glGetnTexImageARB, glGetTextureImage,
>> - * and glGetTexImage.
>> - *
>> - * Requires caller to pass in texImage object because _mesa_GetTextureImage
>> - * must handle the GL_TEXTURE_CUBE_MAP target.
>> - *
>> - * \param target texture target.
>> - * \param level image level.
>> - * \param format pixel data format for returned image.
>> - * \param type pixel data type for returned image.
>> - * \param bufSize size of the pixels data buffer.
>> - * \param pixels returned pixel data.
>> - * \param dsa True when the caller is an ARB_direct_state_access function,
>> - * false otherwise
>> + * Error-check the offset and size arguments to
>> + * glGet[Compressed]TextureSubImage().
>> + * \return true if error, false if no error.
>> */
>> -static void
>> -get_texture_image(struct gl_context *ctx,
>> - struct gl_texture_object *texObj,
>> - struct gl_texture_image *texImage, GLenum target,
>> - GLint level, GLenum format, GLenum type,
>> - GLsizei bufSize, GLvoid *pixels, bool dsa)
>> +static bool
>> +dimensions_error_check(struct gl_context *ctx,
>> + const struct gl_texture_image *texImage,
>> + GLint xoffset, GLint yoffset, GLint zoffset,
>> + GLsizei width, GLsizei height, GLsizei depth,
>> + const char *caller)
>> {
>> - assert(texObj);
>> - assert(texImage);
>> + const GLenum target = texImage->TexObject->Target;
>>
>> - FLUSH_VERTICES(ctx, 0);
>> + switch (target) {
>> + case GL_TEXTURE_1D:
>> + if (yoffset != 0) {
>> + _mesa_error(ctx, GL_INVALID_VALUE,
>> + "%s(1D, yoffset = %d)", caller, yoffset);
>> + return true;
>> + }
>> + if (height != 1) {
>> + _mesa_error(ctx, GL_INVALID_VALUE,
>> + "%s(1D, height = %d)", caller, 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,
>> + "%s(zoffset = %d)", caller, zoffset);
>> + return true;
>> + }
>> + if (depth != 1) {
>> + _mesa_error(ctx, GL_INVALID_VALUE,
>> + "%s(depth = %d)", caller, depth);
>> + return true;
>> + }
>> + break;
>>
>> - /*
>> - * Legal target checking has been moved up to GetnTexImage and
>> - * GetTextureImage so that it can be caught before receiving a NULL
>> - * texImage object and exiting.
>> - */
>> + case GL_TEXTURE_CUBE_MAP:
>> + /* we'll return the faces indicated by the zoffset and depth */
>> + /* XXX */
>
> Did you forget to do something here? [Not sure that you have, but the
> XXX seems bad to have here.]
Left over code fragment - removed in new series.
New patch series coming soon.
-Brian
More information about the mesa-dev
mailing list