[Mesa-dev] [PATCH 02/23] main: Add entry point for TextureBufferRange.

[Martin Peres]

Hey Laura,

Thanks for this code. I'll be sending reviews throughout the week :)

On 12/02/15 04:05, Laura Ekstrand wrote:
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index 336feff..ce6f446 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -5197,6 +5197,96 @@ _mesa_TextureBuffer(GLuint texture, GLenum internalFormat, GLuint buffer)
>                                 bufObj, 0, buffer ? -1 : 0, false, true);
>   }
>   
> +void GLAPIENTRY
> +_mesa_TextureBufferRange(GLuint texture, GLenum internalFormat, GLuint buffer,
> +                         GLintptr offset, GLsizeiptr size)
> +{
> +   struct gl_texture_object *texObj;
> +   struct gl_buffer_object *bufObj;
> +
> +   GET_CURRENT_CONTEXT(ctx);
> +
> +   /* NOTE: ARB_texture_buffer_object has interactions with
> +    * the compatibility profile that are not implemented.
> +    */
> +   if (!(ctx->API == API_OPENGL_CORE &&
> +         ctx->Extensions.ARB_texture_buffer_object)) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> +                  "glTextureBufferRange(ARB_texture_buffer_object is not"
> +                  " implemented for the compatibility profile)");
> +      return;
> +   }

Can you point me to the relevant part of the spec that explains the 
interaction?

Did you simply took this code out of _mesa_TexBufferRange?
> +
> +   bufObj = _mesa_lookup_bufferobj(ctx, buffer);
> +   if (bufObj) {
> +      /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
> +       * Textures:
> +       *    "An INVALID_VALUE error is generated if offset is negative, if
> +       *    size is less than or equal to zero, or if offset + size is greater
> +       *    than the value of BUFFER_SIZE for the buffer bound to target."
> +       */
> +      if (offset < 0) {
> +         _mesa_error(ctx, GL_INVALID_VALUE,
> +                     "glTextureBufferRange(offset %d < 0)", (int) offset);
> +         return;
> +      }
> +
> +      if (size <= 0) {
> +         _mesa_error(ctx, GL_INVALID_VALUE,
> +                     "glTextureBufferRange(size %d <= 0)", (int) size);
> +         return;
> +      }
> +
> +      if (offset + size > bufObj->Size) {
> +         _mesa_error(ctx, GL_INVALID_VALUE,
> +                     "glTextureBufferRange("
> +                     "offset %d + size %d > buffer_size %d)", (int) offset,
> +                     (int) size, (int) bufObj->Size);
> +         return;
> +      }
> +
> +      /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
> +       * Textures:
> +       *    "An INVALID_VALUE error is generated if offset is not an integer
> +       *    multiple of the value of TEXTURE_BUFFER_OFFSET_ALIGNMENT."
> +       */
> +      if (offset % ctx->Const.TextureBufferOffsetAlignment) {
> +         _mesa_error(ctx, GL_INVALID_VALUE,
> +                     "glTextureBufferRange(invalid offset alignment)");
> +         return;
> +      }
> +   } else if (buffer) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> +                  "glTextureBufferRange(unrecognized buffer %u)", buffer);
> +      return;
> +   } else {
> +
> +      /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
> +       * Textures:
> +       *    "If buffer is zero, then any buffer object attached to the buffer
> +       *    texture is detached, the values offset and size are ignored and
> +       *    the state for offset and size for the buffer texture are reset to
> +       *    zero."
> +       */
> +      offset = 0;
> +      size = 0;
> +   }
> +
> +   /* Get the texture object by Name. */
> +   texObj = _mesa_lookup_texture_err(ctx, texture, "glTextureBufferRange");
> +   if (!texObj)
> +      return;

This should return INVALID_OPERATION:

"An INVALID_OPERATION error is generated by TextureBufferRange if 
texture is not the name of an existing texture object."
> +
> +   if (texObj->Target != GL_TEXTURE_BUFFER_ARB) {
> +      _mesa_error(ctx, GL_INVALID_ENUM, "glTextureBufferRange(texture target"
> +                                        " is not GL_TEXTURE_BUFFER)");
> +      return;
> +   }

What are you trying to do here? First, I see no mention of this in the 
spec (but I am quite possibly wrong).

Secondly, you are checking the target before calling 
_mesa_texture_buffer_range which does not care about the target.

Is it a copy/paste from _mesa_TextureBuffer? Please explain why this is 
needed since I surely have not read the spec
as closely as you did on this matter.