[Mesa-dev] [PATCH 4/9] mesa: add helper convenience functions for fetching geometry of gl_framebuffer

[Rogovin, Kevin]

> I'm waffling about this a bit.  I'm concerned that people will use
> buffer->Width when they should use the accessor.  I don't see any
> changes to core Mesa code to use the accessor, so I'm a little concerned that some subtle, incorrect behavior is introduced... but there may well not be.

> A common idiom in Mesa is to have an _Value field that is the derived value.  In many structures in mtypes.h you can see things like Enabled and _ReallyEnabled.  One is what the API sets, and the other is what the driver uses that is based on the API setting and "other factors."  In many ways, the existing gl_framebuffer Width and Height fields are already derived values based on the sizes of the attachments.

>For at least Width, Height, and MaxNumLayers, it seems better to set the existing fields differently when _HasAttachments is true.  That reduces the number of places where a person has to think about _HasAttachments considerably... though perhaps there is something else that I'm not thinking of?

I was thinking of doing this when I was making the patch. The problem is that Width, Height and MaxNumLayers were used for two different purposes:
 1. Specifying the intersection of the attached buffers (this affects blitting for example)
 2. Specifying the "geometry" to send to a rasterizer

Changing Width and Height to be the geometry made me itch because Width and Height are zero when there are no buffer attachments. I added a mess of comments about the meaning of Width and Height. My concern is that other parts of code use Width and Height being 0 to do other things.  With this patch way, the meaning of Width and Height has not changed; instead a diver has to "know" what it is doing when it enables the extension.

I am not particular about this though; I made it this way in an attempt to reduce the scope of changes from this patch series. If people want that the meaning of Width, Height and MaxSamplers changes I can go with that too, I just think it can increase the ickiness of making sure no unintended side effects are added is ickier.


> If we go that route, we should probably just add a gl_framebuffer::Samples field for uniformity.

> +
> +
> +static inline  GLuint
> +_mesa_geometric_height(const struct gl_framebuffer *buffer) {
> +   return buffer->_HasAttachments ?
> +      buffer->Height : buffer->DefaultGeometry.Height; }
> +
> +static inline  GLuint
> +_mesa_geometric_samples(const struct gl_framebuffer *buffer) {
> +   return buffer->_HasAttachments ?
> +      buffer->Visual.samples : buffer->DefaultGeometry.NumSamples;
> +}
> +
> +static inline  GLuint
> +_mesa_geometric_layers(const struct gl_framebuffer *buffer) {
> +   return buffer->_HasAttachments ?
> +      buffer->MaxNumLayers : buffer->DefaultGeometry.Layers; }
> +
>  extern void
>  _mesa_update_draw_buffer_bounds(struct gl_context *ctx);
>  
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h index 
> ef97538..f0e8fbc 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -3178,7 +3178,13 @@ struct gl_framebuffer
>      * the values in DefaultGeometry to initialize its
>      * viewport, scissor and so on (in particular _Xmin,
>      * _Xmax, _Ymin and _Ymax do NOT take into account
> -    * _HasAttachments being false)
> +    * _HasAttachments being false). To get the geometry
> +    * of the framebuffer, the  helper functions
> +    *   _mesa_geometric_width(),
> +    *   _mesa_geometric_height(),
> +    *   _mesa_geometric_samples(),
> +    *   _mesa_geometric_layers()
> +    * are available that check _HasAttachments.
>      */
>     GLboolean _HasAttachments;
>  
>