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