[Mesa-dev] [PATCH] mesa/main: add ARB_clear_texture entrypoints
Brian Paul
brianp at vmware.com
Mon Mar 3 10:06:34 PST 2014
On 03/03/2014 10:52 AM, Ilia Mirkin wrote:
> 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="https://urldefense.proofpoint.com/v1/url?u=http://www.w3.org/2001/XInclude&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=lGQMzzTgII0I7jefp2FHq7WtZ%2BTLs8wadB%2BiIj9xpBY%3D%0A&m=uok%2F%2Bm%2BoqGmkIJzlSaUCqO9rJYPXrJaU%2BDPFlPcxZRU%3D%0A&s=caf1a75e90d0a7631a654c08c1aae5d2161a97a5acb30035dea23830d0f67fbd"/>
>>> +
>>> <!-- 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.)
Yeah, I was suggesting the nested ifs. I like the idea of being
efficient here in any case.
> 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,
If you do switch to else-if, let's at least
assert(_mesa_is_color_format(texImage->_BaseFormat)) to be sure we're
handling all base formats correctly.
> """
> An INVALID_OPERATION error is generated if the base internal format is
> RGBA and the <format> is DEPTH_COMPONENT, STENCIL_INDEX, or DEPTH_STENCIL.
> """
When the spec says RGBA here it corresponds to RGBA, RGB, RG, R, ALPHA,
LUMINANCE, etc. _BaseFormat in Mesa.
>
>>
>>
>>
>>> + (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).
I think the conversion from the user's format/type to the actual texture
format would happen just like glTexSubImage(), with the same format
compatibility checks.
> My goal is to make this work on nouveau first, and post that, and we
> can go from there.
Sounds great.
> Thanks for the review so far!
Thanks for working on the extension!
-Brian
More information about the mesa-dev
mailing list