[Mesa-dev] [PATCH] gallium/util: Fix off-by-one in box intersection

[Gurchetan Singh]

On Thu, Feb 28, 2019 at 12:39 AM Boris Brezillon
<boris.brezillon at collabora.com> wrote:
>
> Hello Gurchetan,
>
> On Wed, 27 Feb 2019 10:34:26 -0800
> Gurchetan Singh <gurchetansingh at chromium.org> wrote:
>
> > On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon
> > <boris.brezillon at collabora.com> wrote:
> > >
> > > From: Daniel Stone <daniels at collabora.com>
> > >
> > > pipe_boxes are x/y + width/height, rather than x0/y0 -> x1/y1. This
> > > means that (x+width) is not included in the box.
> > >
> > > The box intersection check was seemingly written for inclusive regions,
> > > and would falsely assert that adjacent boxes would overlap.
> > >
> > > Fix the off-by-one by being one pixel less greedy.
> >
> > Is there a reason for this change?  I only see this used in a warning
> > in the nine state tracker and virgl (where reporting adjacent
> > intersections is preferred).
>
> This patch was part of a series Daniel worked on to optimize texture
> atlas updates on Vivante GPUs [1]. In the end, this work has been put
> on hold because the perf optimization was not as high as expected, but
> it might be resurrected at some point.
> Anyway, back to the point. In this patchset, the pipe_region_overlaps()
> helper needs to check when regions overlap and not when they're
> adjacent. If other users need u_box_test_intersection_2d() to also
> detect when boxes are adjacent, then we should definitely keep the code
> unchanged, but maybe it's worth a comment in the code to clarify the
> behavior.

Thanks for the information.  You can just modify this function to be
something like:

u_box_test_intersection_2d(const struct pipe_box *a, const struct
pipe_box *b, boolean adjacent_allowed)

[or add another function --- whatever you prefer]

That way we can keep behavior for virgl/nine unchanged.

>
> Regards,
>
> Boris
>
> [1]https://gitlab.collabora.com/bbrezillon/mesa/commits/etna-texture-atlas-18.2.4
>
> >
> > >
> > > Signed-off-by: Daniel Stone <daniels at collabora.com>
> > > Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> > > ---
> > >  src/gallium/auxiliary/util/u_box.h | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/gallium/auxiliary/util/u_box.h b/src/gallium/auxiliary/util/u_box.h
> > > index b3f478e7bfc4..ead7189ecaf8 100644
> > > --- a/src/gallium/auxiliary/util/u_box.h
> > > +++ b/src/gallium/auxiliary/util/u_box.h
> > > @@ -161,15 +161,15 @@ u_box_test_intersection_2d(const struct pipe_box *a,
> > >     unsigned i;
> > >     int a_l[2], a_r[2], b_l[2], b_r[2];
> > >
> > > -   a_l[0] = MIN2(a->x, a->x + a->width);
> > > -   a_r[0] = MAX2(a->x, a->x + a->width);
> > > -   a_l[1] = MIN2(a->y, a->y + a->height);
> > > -   a_r[1] = MAX2(a->y, a->y + a->height);
> > > +   a_l[0] = MIN2(a->x, a->x + a->width - 1);
> > > +   a_r[0] = MAX2(a->x, a->x + a->width - 1);
> > > +   a_l[1] = MIN2(a->y, a->y + a->height - 1);
> > > +   a_r[1] = MAX2(a->y, a->y + a->height - 1);
> > >
> > > -   b_l[0] = MIN2(b->x, b->x + b->width);
> > > -   b_r[0] = MAX2(b->x, b->x + b->width);
> > > -   b_l[1] = MIN2(b->y, b->y + b->height);
> > > -   b_r[1] = MAX2(b->y, b->y + b->height);
> > > +   b_l[0] = MIN2(b->x, b->x + b->width - 1);
> > > +   b_r[0] = MAX2(b->x, b->x + b->width - 1);
> > > +   b_l[1] = MIN2(b->y, b->y + b->height - 1);
> > > +   b_r[1] = MAX2(b->y, b->y + b->height - 1);
> > >
> > >     for (i = 0; i < 2; ++i) {
> > >        if (a_l[i] > b_r[i] || a_r[i] < b_l[i])
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>