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

[Rogovin, Kevin]

Sorry, one more question, which I should have asked before, but neglected to:

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

If this is done, then there are two ways to get the value: from the Visual field and from the new field; Naturally, all drivers now use the field from Visual. If gl_framebuffer::Samples or potentially a better named gl_framebuffer::_Samples is added, then should the field be removed from Visual and all references in drivers to use the new field?

-Kevin


-----Original Message-----
From: Rogovin, Kevin 
Sent: Wednesday, May 06, 2015 1:00 PM
To: 'Ian Romanick'; 'mesa-dev at freedesktop.org'
Subject: RE: [Mesa-dev] [PATCH 4/9] mesa: add helper convenience functions for fetching geometry of gl_framebuffer

One more question:
 for patch 2 of the series, there was the request to change the type of _HasAttachments from GLboolean to bool. If the helper functions "survive", should the helper functions return unsigned int instead of GLuint?

-Kevin

-----Original Message-----
From: Rogovin, Kevin 
Sent: Wednesday, May 06, 2015 10:28 AM
To: 'Ian Romanick'; mesa-dev at freedesktop.org
Subject: RE: [Mesa-dev] [PATCH 4/9] mesa: add helper convenience functions for fetching geometry of gl_framebuffer


> 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;
>  
>