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.rogo...@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.roman...@intel.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
