[Mesa-dev] [PATCH] mesa/main: add ARB_clear_texture entrypoints
Ilia Mirkin
imirkin at alum.mit.edu
Mon Mar 3 09:52:32 PST 2014
On Mon, Mar 3, 2014 at 12:37 PM, Brian Paul <brianp at vmware.com> wrote:
> On 03/01/2014 03:09 PM, Ilia Mirkin wrote:
>>
>> This adds enough code to let drivers implement texture clearing, but
>> doesn't actually do that for any of them.
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>
>> Thought I'd give this a shot. Am I on the right track here? Is the dd API
>> reasonable? I haven't written a piglit test for all the "generic" error
>> cases
>> yet, but I'll get to that in a bit. I'm a little confused between all the
>> different formats on a teximage... BaseFormat appears to be some
>> generalization of the format, but what's the difference between
>> InternalFormat
>> and TexFormat?
>
>
> InternalFormat is what the user requested when the texture was created, such
> as GL_RGB565 or GL_DEPTH_COMPONENT24. BaseFormat is the internalFormat
> boiled down to a basic format, such as GL_RGB or GL_DEPTH_COMPONENT. The
> BaseFormat was originally used for applying the glTexEnv mode. It's also
> used to check for compatibility of src/dest surfaces for glCopyTexImage,
> etc.
>
>
>
>
>> Just the type, but otherwise equivalent? Or are they
>> representing different things?
>>
>> BTW, this is the first time I'm adding stuff to mesa/main, so forgive me
>> if I
>> left out a step.
>>
>> src/mapi/glapi/gen/ARB_clear_texture.xml | 34 ++++++++
>> src/mapi/glapi/gen/gl_API.xml | 2 +
>> src/mesa/main/dd.h | 11 +++
>> src/mesa/main/formatquery.c | 9 +++
>> src/mesa/main/mtypes.h | 1 +
>> src/mesa/main/tests/dispatch_sanity.cpp | 4 +
>> src/mesa/main/teximage.c | 131
>> +++++++++++++++++++++++++++++++
>> src/mesa/main/teximage.h | 10 +++
>> 8 files changed, 202 insertions(+)
>> create mode 100644 src/mapi/glapi/gen/ARB_clear_texture.xml
>>
>> diff --git a/src/mapi/glapi/gen/ARB_clear_texture.xml
>> b/src/mapi/glapi/gen/ARB_clear_texture.xml
>> new file mode 100644
>> index 0000000..bd9116f
>> --- /dev/null
>> +++ b/src/mapi/glapi/gen/ARB_clear_texture.xml
>> @@ -0,0 +1,34 @@
>> +<?xml version="1.0"?>
>> +<!DOCTYPE OpenGLAPI SYSTEM "gl_API.dtd">
>> +
>> +<OpenGLAPI>
>> +
>> +<category name="GL_ARB_clear_texture" number="145">
>> +
>> + <enum name="CLEAR_TEXTURE" value="0x9365"/>
>> +
>> + <function name ="ClearTexImage" offset="assign">
>> + <param name="texture" type="GLuint"/>
>> + <param name="level" type="GLint"/>
>> + <param name="format" type="GLenum"/>
>> + <param name="type" type="GLenum"/>
>> + <param name="data" type="const GLvoid *"/>
>
>
> s/GLvoid/void/ The ARB has deprecated the GLvoid type.
>
>
>
>> + </function>
>> +
>> + <function name ="ClearTexSubImage" offset="assign">
>> + <param name="texture" type="GLuint"/>
>> + <param name="level" type="GLint"/>
>> + <param name="xoffset" type="GLint"/>
>> + <param name="yoffset" type="GLint"/>
>> + <param name="zoffset" type="GLint"/>
>> + <param name="width" type="GLsizei"/>
>> + <param name="height" type="GLsizei"/>
>> + <param name="depth" type="GLsizei"/>
>> + <param name="format" type="GLenum"/>
>> + <param name="type" type="GLenum"/>
>> + <param name="data" type="const GLvoid *"/>
>
>
> s/GLvoid/void/
>
>
>
>> + </function>
>> +
>> +</category>
>> +
>> +</OpenGLAPI>
>> diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml
>> index 7e1946e..15f8e32 100644
>> --- a/src/mapi/glapi/gen/gl_API.xml
>> +++ b/src/mapi/glapi/gen/gl_API.xml
>> @@ -8515,6 +8515,8 @@
>> </function>
>> </category>
>>
>> +<xi:include href="ARB_clear_texture.xml"
>> xmlns:xi="http://www.w3.org/2001/XInclude"/>
>> +
>> <!-- Non-ARB extensions sorted by extension number. -->
>>
>> <category name="GL_EXT_blend_color" number="2">
>> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
>> index 9715241..d824bca 100644
>> --- a/src/mesa/main/dd.h
>> +++ b/src/mesa/main/dd.h
>> @@ -275,6 +275,17 @@ struct dd_function_table {
>> GLint level, mesa_format format,
>> GLint width, GLint height,
>> GLint depth, GLint border);
>> +
>> +
>> + void (*ClearTexSubImage)(struct gl_context *ctx,
>> + struct gl_texture_image *texImage,
>> + GLint xoffset, GLint yoffset, GLint zoffset,
>> + GLsizei width, GLsizei height, GLsizei depth,
>> + GLenum format, GLenum type, const GLvoid
>> *data);
>
>
> I think we may want to follow the example of ClearBufferSubData() and pass a
> clearValue and clearValueSize instead of format/type/data.
>
>
>
>> +
>> + GLenum (*QuerySupportForClearTex)(struct gl_context *ctx,
>> + GLenum internalFormat);
>
>
> The spec says the GL_CLEAR_TEXTURE query is only supported if
> GL_ARB_internalformat_query2 is supported (and we don't). So I'm not sure
> we need this function at this time.
>
>
>
>> +
>> /*@}*/
>>
>>
>> diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c
>> index 40eca87..7997cb4 100644
>> --- a/src/mesa/main/formatquery.c
>> +++ b/src/mesa/main/formatquery.c
>> @@ -140,6 +140,15 @@ _mesa_GetInternalformativ(GLenum target, GLenum
>> internalformat, GLenum pname,
>> count = 1;
>> break;
>> }
>> + case GL_CLEAR_TEXTURE:
>> + if (ctx->Extensions.ARB_clear_texture) {
>> + const GLenum support = ctx->Driver.QuerySupportForClearTex(
>> + ctx, internalformat);
>> + buffer[0] = (GLint) support;
>> + count = 1;
>> + break;
>> + }
>> + /* fallthrough */
>
>
> I'm not sure this is needed. See above.
Yes, Chris Forbes mentioned that to me as well on IRC. I had assumed
that we already supported query2, but really we only support query(1).
I guess whoever ends up doing query2 will have to worry about this.
>
>
>
>> default:
>> _mesa_error(ctx, GL_INVALID_ENUM,
>> "glGetInternalformativ(pname=%s)",
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index 7246b1e..e4a3837 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -3490,6 +3490,7 @@ struct gl_extensions
>> GLboolean ARB_base_instance;
>> GLboolean ARB_blend_func_extended;
>> GLboolean ARB_buffer_storage;
>> + GLboolean ARB_clear_texture;
>
>
> I suspect that we should be able to implement this for all drivers so this
> flag may not be needed. It's OK for now though.
>
>
>
>> GLboolean ARB_color_buffer_float;
>> GLboolean ARB_compute_shader;
>> GLboolean ARB_conservative_depth;
>> diff --git a/src/mesa/main/tests/dispatch_sanity.cpp
>> b/src/mesa/main/tests/dispatch_sanity.cpp
>> index d1b0011..8690b5d 100644
>> --- a/src/mesa/main/tests/dispatch_sanity.cpp
>> +++ b/src/mesa/main/tests/dispatch_sanity.cpp
>> @@ -936,6 +936,10 @@ const struct function gl_core_functions_possible[] =
>> {
>> /* GL_ARB_buffer_storage */
>> { "glBufferStorage", 43, -1 },
>>
>> + /* GL_ARB_clear_texture */
>> + { "glClearTexImage", 13, -1 },
>> + { "glClearTexSubImage", 13, -1 },
>> +
>> { NULL, 0, -1 }
>> };
>>
>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>> index 0519d22..9171187 100644
>> --- a/src/mesa/main/teximage.c
>> +++ b/src/mesa/main/teximage.c
>> @@ -3768,6 +3768,137 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint
>> level,
>> x, y, width, height);
>> }
>>
>> +static struct gl_texture_image *
>> +get_tex_image(struct gl_context *ctx, GLuint texture, GLint level, GLint
>> zoffset)
>> +{
>> + GLenum target = 0;
>> + struct gl_texture_object *texObj = _mesa_lookup_texture(ctx, texture);
>> + struct gl_texture_image *texImage;
>> +
>> + if (!texture || !texObj) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid texture)");
>> + return NULL;
>> + }
>> +
>> + switch (texObj->Target) {
>> + case GL_TEXTURE_BUFFER:
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(texture can't be buffer)");
>> + return NULL;
>> + case GL_TEXTURE_CUBE_MAP_ARB:
>> + case GL_TEXTURE_CUBE_MAP_ARRAY:
>> + target = GL_TEXTURE_CUBE_MAP_POSITIVE_X + zoffset;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + texImage = _mesa_select_tex_image(ctx, texObj, target, level);
>> + if (!texImage) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(missing image)");
>> + return NULL;
>> + }
>> +
>> + return texImage;
>> +}
>> +
>> +static void
>> +clear_tex_sub_image(struct gl_context *ctx, struct gl_texture_image
>> *texImage,
>> + GLint xoffset, GLint yoffset, GLint zoffset,
>> + GLsizei width, GLsizei height, GLsizei depth,
>> + GLenum format, GLenum type, const void *data)
>> +{
>> + if (!texImage)
>> + return;
>> +
>> + if (_mesa_is_compressed_format(ctx, texImage->InternalFormat)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(compressed format)");
>> + return;
>> + }
>> +
>> + if (texImage->_BaseFormat == GL_DEPTH_COMPONENT &&
>> + format != GL_DEPTH_COMPONENT) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid format)");
>> + return;
>> + }
>> + if (texImage->_BaseFormat == GL_DEPTH_STENCIL &&
>> + format != GL_DEPTH_STENCIL) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid format)");
>> + return;
>> + }
>> + if (texImage->_BaseFormat == GL_STENCIL_INDEX &&
>> + format != GL_STENCIL_INDEX) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid format)");
>> + return;
>> + }
>> + if (texImage->_BaseFormat == GL_RGBA &&
>
>
> _BaseFormat may also be GL_RGB, GL_ALPHA, GL_LUMINANCE, etc.
>
> I think if you used else-ifs above, by time you get here you can be sure the
> _BaseFormat is a color format. Or, you could use
> _mesa_is_color_format(_BaseFormat).
I was just following the conditions in the spec. If I use else's here,
that wouldn't help. The if's only happen if there's an error, and it's
not an error to have a depth/stencil BaseFormat, it's just an error
for certain format/BaseFormat combinations. (I guess I could change
the structure to have nested if's for the error conditions if you
think that's cleaner, but IMO it's not... clearing a texture is a
relatively expensive operation, a few conditionals here and there
won't affect performance.) I'll change this to check for all color
formats with _mesa_is_color_format, I guess that was the intention of
the spec. As is though, it reads,
"""
An INVALID_OPERATION error is generated if the base internal format is
RGBA and the <format> is DEPTH_COMPONENT, STENCIL_INDEX, or DEPTH_STENCIL.
"""
>
>
>
>> + (format == GL_DEPTH_COMPONENT ||
>> + format == GL_DEPTH_STENCIL ||
>> + format == GL_STENCIL_INDEX)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid format)");
>> + return;
>> + }
>> + if (_mesa_is_format_integer_color(texImage->InternalFormat) !=
>> + _mesa_is_format_integer_color(format)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid format)");
>> + return;
>> + }
>> +
>> + if (xoffset < -texImage->Border ||
>> + xoffset + width > texImage->Width - texImage->Border ||
>> + yoffset < -texImage->Border ||
>> + yoffset + height > texImage->Height - texImage->Border ||
>> + zoffset < -texImage->Border ||
>> + zoffset + depth > texImage->Depth - texImage->Border) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glClearTexSubImage(invalid bounds)");
>> + }
>> +
>> + ctx->Driver.ClearTexSubImage(ctx, texImage, xoffset, yoffset, zoffset,
>> + width, height, depth, format, type,
>> data);
>
>
> If we change the driver function to take clearValue and clearValueSize,
> you'd use the convert_clear_buffer_data() function to convert the clear
> value from the user's format to the texture mesa_format.
>
>
>
>> +}
>> +
>> +void GLAPIENTRY
>> +_mesa_ClearTexSubImage( GLuint texture, GLint level,
>> + GLint xoffset, GLint yoffset, GLint zoffset,
>> + GLsizei width, GLsizei height, GLsizei depth,
>> + GLenum format, GLenum type, const void *data )
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> + struct gl_texture_image *texImage =
>> + get_tex_image(ctx, texture, level, zoffset);
>
>
> check if texImage is NULL here, as you do below.
>
>
>
>> +
>> + clear_tex_sub_image(ctx, texImage, xoffset, yoffset, zoffset,
>> + width, height, depth, format, type, data);
>> +}
>> +
>> +void GLAPIENTRY
>> +_mesa_ClearTexImage( GLuint texture, GLint level,
>> + GLenum format, GLenum type, const void *data )
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> +
>> + struct gl_texture_image *texImage
>> + = get_tex_image(ctx, texture, level, 0);
>> + if (!texImage)
>> + return;
>> +
>> + /* XXX figure out which dimensions the texImage has, and pass in 0's
>> for
>> + * the border.
>> + */
>> + clear_tex_sub_image(ctx, texImage,
>> + -texImage->Border, -texImage->Border,
>> -texImage->Border,
>> + texImage->Width, texImage->Height,
>> texImage->Depth,
>
>
> Even though we don't really support borders anymore, I believe this should
> be:
>
> texImage->Width + 2 * texImage->Boarder, etc.
>
>
>
>> + format, type, data);
>> +}
>>
>>
>>
>> diff --git a/src/mesa/main/teximage.h b/src/mesa/main/teximage.h
>> index 5f8a477..4d01ff9 100644
>> --- a/src/mesa/main/teximage.h
>> +++ b/src/mesa/main/teximage.h
>> @@ -261,6 +261,16 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint level,
>>
>>
>> extern void GLAPIENTRY
>> +_mesa_ClearTexSubImage( GLuint texture, GLint level,
>> + GLint xoffset, GLint yoffset, GLint zoffset,
>> + GLsizei width, GLsizei height, GLsizei depth,
>> + GLenum format, GLenum type, const void *data );
>> +
>> +extern void GLAPIENTRY
>> +_mesa_ClearTexImage( GLuint texture, GLint level,
>> + GLenum format, GLenum type, const void *data );
>> +
>> +extern void GLAPIENTRY
>> _mesa_CompressedTexImage1D(GLenum target, GLint level,
>> GLenum internalformat, GLsizei width,
>> GLint border, GLsizei imageSize,
>>
>
>
> This looks good in general. I think you should proceed with fallback
> tex-clear functions for the state tracker and legacy drivers to flesh out
> the clearValue/clearValueSize issue.
Yes, I've been struggling a bit with the correct APIs there. I'll take
a look at what ClearBufferSubData does. One complication here is that
the format of the data need not be the format of the texture (as I
understand it, one could easily have a R5G6B5_UINT texture and pass in
a R32G32B32A32_FLOAT clear value). It's unclear to me if the proper
API is to pass in some standardized color thing, or to convert it to
the right format first (and if so, how? I'm doing it in the st right
now with util_translate_format, but that won't work for classic
drivers).
My goal is to make this work on nouveau first, and post that, and we
can go from there.
Thanks for the review so far!
-ilia
More information about the mesa-dev
mailing list