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

[Ian Romanick]

On 02/17/2015 07:59 AM, Martin Peres wrote:
> 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?

In compatibility profile, TexBos can support luminance, luminance-alpha,
and intensity textures.  Intel hardware doesn't support that, and none
of the TexBo common code supports it either.

But... at least for glTexBuffer and glTexBufferRange there is (almost) a
better way to do this.  We should change the GL API generator code so
that we can omit functions from compatibility profile.  We can already
do this for core profile (look for the deprecated='3.1' bits in
src/mapi/glapi/gen).

Due to previously mentioned issues with libraries like GLEW, we may not
be able to do this with DSA functions that are core-only if we're going
to expose DSA on compatibility profile.

> 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.
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>