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

[Ian Romanick]

On 04/29/2015 01:56 AM, kevin.rogovin at intel.com wrote:
> From: Kevin Rogovin <kevin.rogovin at intel.com>
> 
> Add convenience helper functions for fetching geometry of gl_framebuffer
> that return the geometry of the gl_framebuffer instead of the geometry of
> the buffers of the gl_framebuffer when then the gl_framebuffer has no 
> attachments.
> 
> ---
>  src/mesa/main/framebuffer.h | 29 +++++++++++++++++++++++++++++
>  src/mesa/main/mtypes.h      |  8 +++++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/framebuffer.h b/src/mesa/main/framebuffer.h
> index a427421..4a2e0d9 100644
> --- a/src/mesa/main/framebuffer.h
> +++ b/src/mesa/main/framebuffer.h
> @@ -76,6 +76,35 @@ _mesa_scissor_bounding_box(const struct gl_context *ctx,
>                             const struct gl_framebuffer *buffer,
>                             unsigned idx, int *bbox);
>  
> +static inline  GLuint
> +_mesa_geometric_width(const struct gl_framebuffer *buffer)
> +{
> +   return buffer->_HasAttachments ?
> +      buffer->Width : buffer->DefaultGeometry.Width;
> +}

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?

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