[Mesa-dev] [PATCH 4/5] mesa: Implement functions for clear_buffer_object extensions
Brian Paul
brianp at vmware.com
Tue Dec 10 08:44:11 PST 2013
On 12/10/2013 06:13 AM, Pi Tabred wrote:
> - _mesa_buffer_clear_subdata: default callback for dd function table
> - _mesa_ClearBufferData: API function
> - _mesa_ClearBufferSubData: API function
>
> *NOTE* According to the spec, it should be possible to clear some part of
> a buffer, even if a different, non-overlapping part is mapped, this is currently
> not possible. It was suggested to implement ClearBufferSubData using
> MapBufferRange. However, this does not work if a part of the buffer is already
> mapped. I am open for suggestions.
We should probably discuss this on the list to get some ideas. I
haven't read the extension spec yet.
> Modify get_buffer to return the correct error for this extension
> ---
> src/mesa/main/bufferobj.c | 206 +++++++++++++++++++++++++++++++++++++++++++++-
> src/mesa/main/bufferobj.h | 4 +
> 2 files changed, 209 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> index bfeed83..66d6ad4 100644
> --- a/src/mesa/main/bufferobj.c
> +++ b/src/mesa/main/bufferobj.c
> @@ -41,6 +41,9 @@
> #include "fbobject.h"
> #include "mtypes.h"
> #include "texobj.h"
> +#include "teximage.h"
> +#include "glformats.h"
> +#include "texstore.h"
> #include "transformfeedback.h"
> #include "dispatch.h"
>
> @@ -138,7 +141,13 @@ get_buffer(struct gl_context *ctx, const char *func, GLenum target)
> }
>
> if (!_mesa_is_bufferobj(*bufObj)) {
> - _mesa_error(ctx, GL_INVALID_OPERATION, "%s(buffer 0)", func);
> + if (strcmp(func, "glClearBufferSubData\0") == 0 ||
> + strcmp(func, "glClearBufferData\0") == 0) {
> + _mesa_error(ctx, GL_INVALID_VALUE, "%s(no buffer bound)", func);
> + }
> + else {
> + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(buffer 0)", func);
> + }
> return NULL;
> }
Instead of strcmp, it might be better to simply add an error parameter
to the function (either GL_INVALID_VALUE or OPERATION) indicating which
error should be generated depending on the caller.
>
> @@ -539,6 +548,64 @@ _mesa_buffer_get_subdata( struct gl_context *ctx, GLintptrARB offset,
>
>
> /**
> + * Clear a subrange of buffer object with supplied data. If the data range
> + * specified by \c size + \c offset extends beyond the end of the buffer
> + * the error INVALID_VALUE is generated, if data is NULL the buffer is filled
> + * with zeros.
> + *
> + * This is the default callback for \c dd_function_table::ClearBufferSubData()
> + * Note that all GL error checking will have been done already.
> + *
> + * \param ctx GL context.
> + * \param internalformat Internal format of the buffer data.
> + * \param offset Offset of the first byte to be cleared.
> + * \param size Size, in bytes, of the data range.
> + * \param format Format of the supplied data.
> + * \param type Type of the supplied data.
> + * \param data Source of the data.
> + * \param bufObj Object to be used.
> + *
> + * \sa glClearBufferSubDataARB, dd_function_table::ClearBufferSubData.
> + */
> +static void
> +_mesa_buffer_clear_subdata( struct gl_context *ctx, GLenum internalformat,
> + GLintptr offset, GLsizeiptr size,
> + GLenum format, GLenum type,
> + const GLvoid *data, struct gl_buffer_object *bufObj )
Remove space after ( and before ).
> +{
> + ASSERT(ctx->Driver.MapBufferRange);
> + void *dest = ctx->Driver.MapBufferRange(ctx, offset, size,
> + GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT,
> + bufObj);
> +
> + if (data == NULL)
> + {
Brace on preceding line.
> + memset(dest, 0, size);
> + ctx->Driver.UnmapBuffer(ctx, bufObj);
> + return;
> + }
> +
> + gl_format formatMesa = _mesa_validate_texbuffer_format(ctx, internalformat);
> + GLenum formatBase = _mesa_get_format_base_format(formatMesa);
Variables must be declared before code for MSVC's sake (it doesn't
handle that C99 convention). This needs fixing throughout both functions.
> +
> + size_t sizeOfFormat = _mesa_get_format_bytes(formatMesa);
> +
> + GLubyte* src = malloc(sizeOfFormat);
Should probably have a src==NULL check here to silence static analysis
tools.
> + _mesa_texstore(ctx, 1, formatBase, formatMesa, 0,
> + &src, 1, 1, 1, format, type, data, &ctx->Unpack);
> +
> + for (int i = 0; i < size/sizeOfFormat; ++i)
We need to declare the variable 'i' before the loop.
> + {
> + memcpy(dest, src, sizeOfFormat);
> + dest += sizeOfFormat;
> + }
> + ctx->Driver.UnmapBuffer(ctx, bufObj);
> +
> + free(src);
> +}
> +
> +
> +/**
> * Default fallback for \c dd_function_table::MapBufferRange().
> * Called via glMapBufferRange().
> */
> @@ -844,6 +911,9 @@ _mesa_init_buffer_object_functions(struct dd_function_table *driver)
> driver->GetBufferSubData = _mesa_buffer_get_subdata;
> driver->UnmapBuffer = _mesa_buffer_unmap;
>
> + /* GL_ARB_clear_buffer_object */
> + driver->ClearBufferSubData = _mesa_buffer_clear_subdata;
> +
> /* GL_ARB_map_buffer_range */
> driver->MapBufferRange = _mesa_buffer_map_range;
> driver->FlushMappedBufferRange = _mesa_buffer_flush_mapped_range;
> @@ -1172,6 +1242,140 @@ _mesa_GetBufferSubData(GLenum target, GLintptrARB offset,
> }
>
>
> +void GLAPIENTRY
> +_mesa_ClearBufferData(GLenum target, GLenum internalformat, GLenum format,
> + GLenum type, const GLvoid * data)
> +{
> + GET_CURRENT_CONTEXT(ctx);
> +
> + gl_format internalFormatMesa = _mesa_validate_texbuffer_format(ctx, internalformat);
> + if (internalFormatMesa == MESA_FORMAT_NONE) {
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glClearBufferData(invalid internalformat)");
> + return;
> + }
> +
> + /* NOTE: not mentioned in ARB_clear_buffer_object but according to
> + * EXT_texture_integer there is no conversion between integer and
> + * non-integer formats
> + */
> + if (_mesa_is_color_format(internalformat)) {
> + if (_mesa_is_enum_format_signed_int(format)
> + !=
> + _mesa_is_format_integer_color(internalFormatMesa)) {
I'd put the != and next line together.
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glClearBufferData(integer vs non-integer)");
> + return;
Please use spaces, not tabs for indenting.
> + }
> + }
> +
> + if (!_mesa_is_color_format(format)) {
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glClearBufferData(no color format)");
> + }
Are you sure about that error? Where is this mentioned in the spec?
Plus, you're missing a 'return' statement.
> +
> + struct gl_buffer_object* bufObj = get_buffer(ctx, "glClearBufferData", target);
> + if (!bufObj) {
> + return;
> + }
> +
> + GLenum errorFormatType = _mesa_error_check_format_and_type(ctx, format, type);
> + if (errorFormatType != GL_NO_ERROR) {
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glClearBufferData(invalid format or type)");
> + return;
> + }
> +
> + GLint sizeOfFormat = _mesa_get_format_bytes(internalFormatMesa);
> + if (bufObj->Size % sizeOfFormat != 0) {
> + _mesa_error(ctx, GL_INVALID_VALUE,
> + "glClearBufferData(buffer size no multiple of internalformat size)");
"size is not a multiple of the internalformat size"
> + }
> +
> + if (_mesa_bufferobj_mapped(bufObj)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glClearBufferData(buffer currently mapped)");
> + return;
> + }
> +
> + ctx->Driver.ClearBufferSubData( ctx, internalformat, 0, bufObj->Size,
> + format, type, data, bufObj );
> +}
> +
> +
> +void GLAPIENTRY
> +_mesa_ClearBufferSubData(GLenum target, GLenum internalformat, GLintptr offset,
> + GLsizeiptr size, GLenum format, GLenum type,
> + const GLvoid * data)
> +{
> + GET_CURRENT_CONTEXT(ctx);
> +
> + gl_format internalFormatMesa = _mesa_validate_texbuffer_format(ctx, internalformat);
> + if (internalFormatMesa == MESA_FORMAT_NONE) {
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glClearBufferSubData(invalid internalformat)");
> + return;
> + }
> +
> + if (_mesa_is_color_format(internalformat)) {
> + if (_mesa_is_enum_format_signed_int(format)
> + !=
> + _mesa_is_format_integer_color(internalFormatMesa)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glClearBufferData(integer vs non-integer)");
> + return;
> + }
> + }
> +
> + if (!_mesa_is_color_format(format)) {
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glClearBufferSubData(no color format)");
> + }
> +
> + struct gl_buffer_object* bufObj = get_buffer(ctx, "glClearBufferSubData",
> + target);
> + if (!bufObj) {
> + return;
> + }
> +
> + GLenum errorFormatType = _mesa_error_check_format_and_type(ctx, format, type);
> + if (errorFormatType != GL_NO_ERROR) {
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glClearBufferSubData(invalid format or type)");
> + return;
> + }
> +
> + if (offset < 0 || size < 0) {
> + _mesa_error(ctx, GL_INVALID_VALUE,
> + "glClearBufferSubData(offset or size less than zero)");
> + return;
> + }
> +
> + if (bufObj->Size < offset + size) {
> + _mesa_error(ctx, GL_INVALID_VALUE, "glClearBufferSubData(range error)");
> + }
> +
> + GLint sizeOfFormat = _mesa_get_format_bytes(internalFormatMesa);
> + if (offset % sizeOfFormat != 0 || size % sizeOfFormat != 0) {
> + _mesa_error(ctx, GL_INVALID_VALUE,
> + "glClearBufferSubData(offset or size no multiple of internalformat size)");
> + }
> +
> + if (_mesa_bufferobj_mapped(bufObj)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glClearBufferSubData(intersection with mapped range)");
> + return;
> + }
> +/* if (_mesa_bufferobj_range_mapped(bufObj, offset, size)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glClearBufferSubData(intersection with mapped range)");
> + }*/
> +
> + ctx->Driver.ClearBufferSubData( ctx, internalformat, offset, size,
> + format, type, data, bufObj );
> +}
It looks like a lot of the error checking in these two functions is the
same. You could probably move all that into a shared error-checking
helper function.
> +
> +
> void * GLAPIENTRY
> _mesa_MapBuffer(GLenum target, GLenum access)
> {
> diff --git a/src/mesa/main/bufferobj.h b/src/mesa/main/bufferobj.h
> index 0b898a2..b38519f 100644
> --- a/src/mesa/main/bufferobj.h
> +++ b/src/mesa/main/bufferobj.h
> @@ -120,6 +120,10 @@ void GLAPIENTRY
> _mesa_BufferSubData(GLenum target, GLintptrARB offset, GLsizeiptrARB size, const GLvoid * data);
> void GLAPIENTRY
> _mesa_GetBufferSubData(GLenum target, GLintptrARB offset, GLsizeiptrARB size, void * data);
> +void GLAPIENTRY
> +_mesa_ClearBufferData(GLenum target, GLenum internalformat, GLenum format, GLenum type, const GLvoid * data);
> +void GLAPIENTRY
> +_mesa_ClearBufferSubData(GLenum target, GLenum internalformat, GLintptr offset, GLsizeiptr size, GLenum format, GLenum type, const GLvoid * data);
> void * GLAPIENTRY
> _mesa_MapBuffer(GLenum target, GLenum access);
> GLboolean GLAPIENTRY
>
That header file could use some cleanup (space between lines, 78-column
wrapping, etc) but that's OK for now.
More information about the mesa-dev
mailing list