[Mesa-dev] [PATCH] Implement the ARB_clear_texture extension

[Neil Roberts]

Thanks for the review.

I'm working on incorporating the changes into a v2 patch series. However
I'm a bit stuck with what to do about enabling the extension assuming
there is no Gallium implementation for now. I couldn't find a convenient
place to enable the extension for all DRI-based drivers without also
enabling it for Gallium. Is there such a place? If not, maybe I should
add one? It seems like it would be a bit silly to expiclitly enable it
in all of the DRI drivers when the function pointers are filled in in
the generic _mesa_init_driver_functions function and the implementation
is entirely driver-independent.

Ilia Mirkin writes:

> You might want to check out the impl I did a few months ago --
> https://github.com/imirkin/mesa/commits/clear_texture . It went a few
> rounds of review, but died at the "there are no piglit tests" stage.

Ah, sorry, I wasn't aware that you had already started working on this.
I think for the v2 patch series it would make sense to base the patches
on at least the first patch in your series.

> I believe the usual thing is to split this sort of thing up into a few
> patches, roughly how I had it.

Yes, ok, that makes sense.

> I think you're still missing some api-errors and try-all-the-formats
> formats piglit tests. Especially non-renderable formats and MS ones.

I've posted a bunch more tests to the piglit mailing list including
various texture formats and multi-sample and depth-stencil textures. I
guess I still need to add the error checking tests.

>> +   if (ctx->Version >= 30 || ctx->Extensions.EXT_texture_integer) {
>> +      /* both source and dest must be integer-valued, or neither */
>> +      if (_mesa_is_format_integer_color(texImage->TexFormat) !=
>> +          _mesa_is_enum_format_integer(format)) {
>
> I had used _mesa_is_enum_format_integer_or_type() here.

I copied this snippet from texsubimage_error_check(). The
_mesa_is_enum_format_or_type_integer function looks strange. For example
it returns TRUE if the type is GL_UNSIGNED_BYTE regardless of the
format. I think the idea is to try and check when the format isn't
normalised. Wouldn't this return the wrong thing for GL_RGBA with
GL_UNSIGNED_BYTE (ie, that format should be normalized)? It doesn't look
like the function is used anywhere else.

> Also, is it possible to have a ctx->Version >= 30 &&
> !EXT_texture_integer?

No, but it is possible to to have EXT_texture_integer with
ctx->Version<30 which presumably is what the expression is trying to
catch.

> FWIW I also had explicit checks making sure that various depth/stencil
> stuff weren't mixed together improperly (i.e. passing a
> GL_DEPTH_COMPONENT format for a tex image with a diff base format,
> etc). Perhaps this checking is handled somewhere else in your code and
> I didn't see it though.

My assumption was that _mesa_texstore will take care of catching these
error conditions.

> Wait, you go to all the trouble of computing the "zeroData" but then
> don't use it?

Yes, because I want to check the error conditions even when NULL data is
passed. It would be nice of the spec for the extension said that if the
data is NULL then the format and type are completely ignored and no
error will be signalled, but sadly it doesn't look like that is the
case.

>> +   clearValueSize = _mesa_get_format_bytes(texImage->TexFormat);
>
> Basically every implementation is going to do this... why not just
> pass it in? Seems safer to explicitly say how many bytes of the
> pointer are legal to read. Could be seen as redundant wrt the
> TexFormat though... up to you.

I suppose it doesn't really matter much either way, but I don't think
it's true that every implementation is going to use the clearValueSize.
The FBO/meta implementation I added doesn't use it and it looks like the
Gallium implementation which you wrote doesn't use it either.

Regards,
- Neil