[Mesa-dev] [v4 PATCH 05/10] mesa: helper function for scissor box of gl_framebuffer

[Ian Romanick]

On 05/27/2015 02:49 AM, Kevin Rogovin wrote:
> Add helper convenience function that intersects the scissor values
> against a passed bounding box. In addition, to avoid replicated code,
> make the function _mesa_scissor_bounding_box() use this new function.
> 
> v2:
>  Split from patch "mesa:helper-conveniance functions for drivers to implement ARB_framebuffer_no_attachment".
>  White space and long line fixes.
> 
> v3:
>  No changes.
> 
> v4:
>  No changes.
> 
> 
> Signed-off-by: Kevin Rogovin <kevin.rogovin at intel.com>
> ---
>  src/mesa/main/framebuffer.c | 63 +++++++++++++++++++++++++++++++--------------
>  src/mesa/main/framebuffer.h |  5 ++++
>  2 files changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
> index c2cfb92..dd9e4bc 100644
> --- a/src/mesa/main/framebuffer.c
> +++ b/src/mesa/main/framebuffer.c
> @@ -357,42 +357,38 @@ update_framebuffer_size(struct gl_context *ctx, struct gl_framebuffer *fb)
>  }
>  
>  
> +

Spurious (and incorrect) whitespace change here.

>  /**
> - * Calculate the inclusive bounding box for the scissor of a specific viewport
> + * Given a bounding box, intersect the bounding box with the scissor of
> + * a specified vieport.
>   *
>   * \param ctx     GL context.
> - * \param buffer  Framebuffer to be checked against
>   * \param idx     Index of the desired viewport
>   * \param bbox    Bounding box for the scissored viewport.  Stored as xmin,
>   *                xmax, ymin, ymax.
> - *
> - * \warning This function assumes that the framebuffer dimensions are up to
> - * date (e.g., update_framebuffer_size has been recently called on \c buffer).
> - *
> - * \sa _mesa_clip_to_region
>   */
> -void
> -_mesa_scissor_bounding_box(const struct gl_context *ctx,
> -                           const struct gl_framebuffer *buffer,
> -                           unsigned idx, int *bbox)
> +extern void

Based on other patches in the series, remove "extern" here.

> +_mesa_intersect_scissor_bounding_box(const struct gl_context *ctx,
> +                                     unsigned idx, int *bbox)
>  {
> -   bbox[0] = 0;
> -   bbox[2] = 0;
> -   bbox[1] = buffer->Width;
> -   bbox[3] = buffer->Height;
> -
>     if (ctx->Scissor.EnableFlags & (1u << idx)) {

All of the changes in this function below this point seem unnecessary.
If they are necessary, please explain why.  If they're not, mixing
cosmetic changes and functional changes is bad... don't do it. :)  That
also avoids the issue of where to put the "+".

> +      int xmax, ymax;
> +
> +      xmax = ctx->Scissor.ScissorArray[idx].X
> +        + ctx->Scissor.ScissorArray[idx].Width;
> +      ymax = ctx->Scissor.ScissorArray[idx].Y
> +        + ctx->Scissor.ScissorArray[idx].Height;
>        if (ctx->Scissor.ScissorArray[idx].X > bbox[0]) {
>           bbox[0] = ctx->Scissor.ScissorArray[idx].X;
>        }
>        if (ctx->Scissor.ScissorArray[idx].Y > bbox[2]) {
>           bbox[2] = ctx->Scissor.ScissorArray[idx].Y;
>        }
> -      if (ctx->Scissor.ScissorArray[idx].X + ctx->Scissor.ScissorArray[idx].Width < bbox[1]) {
> -         bbox[1] = ctx->Scissor.ScissorArray[idx].X + ctx->Scissor.ScissorArray[idx].Width;
> +      if (xmax < bbox[1]) {
> +         bbox[1] = xmax;
>        }
> -      if (ctx->Scissor.ScissorArray[idx].Y + ctx->Scissor.ScissorArray[idx].Height < bbox[3]) {
> -         bbox[3] = ctx->Scissor.ScissorArray[idx].Y + ctx->Scissor.ScissorArray[idx].Height;
> +      if (ymax < bbox[3]) {
> +        bbox[3] = ymax;
>        }
>        /* finally, check for empty region */
>        if (bbox[0] > bbox[1]) {
> @@ -402,6 +398,33 @@ _mesa_scissor_bounding_box(const struct gl_context *ctx,
>           bbox[2] = bbox[3];
>        }
>     }
> +}
> +
> +/**
> + * Calculate the inclusive bounding box for the scissor of a specific viewport
> + *
> + * \param ctx     GL context.
> + * \param buffer  Framebuffer to be checked against
> + * \param idx     Index of the desired viewport
> + * \param bbox    Bounding box for the scissored viewport.  Stored as xmin,
> + *                xmax, ymin, ymax.
> + *
> + * \warning This function assumes that the framebuffer dimensions are up to
> + * date (e.g., update_framebuffer_size has been recently called on \c buffer).
> + *
> + * \sa _mesa_clip_to_region
> + */
> +void
> +_mesa_scissor_bounding_box(const struct gl_context *ctx,
> +                           const struct gl_framebuffer *buffer,
> +                           unsigned idx, int *bbox)
> +{
> +   bbox[0] = 0;
> +   bbox[2] = 0;
> +   bbox[1] = buffer->Width;
> +   bbox[3] = buffer->Height;
> +
> +   _mesa_intersect_scissor_bounding_box(ctx, idx, bbox);
>  
>     assert(bbox[0] <= bbox[1]);
>     assert(bbox[2] <= bbox[3]);
> diff --git a/src/mesa/main/framebuffer.h b/src/mesa/main/framebuffer.h
> index 8b2aa34..bb1f2ea 100644
> --- a/src/mesa/main/framebuffer.h
> +++ b/src/mesa/main/framebuffer.h
> @@ -76,6 +76,10 @@ _mesa_scissor_bounding_box(const struct gl_context *ctx,
>                             const struct gl_framebuffer *buffer,
>                             unsigned idx, int *bbox);
>  
> +extern void
> +_mesa_intersect_scissor_bounding_box(const struct gl_context *ctx,
> +                                     unsigned idx, int *bbox);
> +
>  static inline  GLuint
>  _mesa_geometric_width(const struct gl_framebuffer *buffer)
>  {
> @@ -83,6 +87,7 @@ _mesa_geometric_width(const struct gl_framebuffer *buffer)
>        buffer->Width : buffer->DefaultGeometry.Width;
>  }
>  
> +

Spurious whitespace change.

>  static inline  GLuint
>  _mesa_geometric_height(const struct gl_framebuffer *buffer)
>  {
> 

With all of the issues above fixed, this patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>