[Mesa-dev] [PATCH] egl: Fix missing clamping in eglSetDamageRegionKHR

[Ilia Mirkin]

On Sat, Jul 7, 2018 at 11:15 PM, Harish Krupo
<harish.krupo.kps at intel.com> wrote:
> Ilia,
>
> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>
>> On Sat, Jul 7, 2018 at 5:05 PM, Harish Krupo <harish.krupo.kps at intel.com> wrote:
>>> Clamp the x and y co-ordinates of the rectangles.
>>>
>>> Signed-off-by: Harish Krupo <harish.krupo.kps at intel.com>
>>> ---
>>>  src/egl/main/eglapi.c | 23 +++++++++--------------
>>>  1 file changed, 9 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>>> index c110349119..c7d07c4fde 100644
>>> --- a/src/egl/main/eglapi.c
>>> +++ b/src/egl/main/eglapi.c
>>> @@ -1320,9 +1320,7 @@ eglSwapBuffersWithDamageKHR(EGLDisplay dpy, EGLSurface surface,
>>>  }
>>>
>>>  /**
>>> - * If the width of the passed rect is greater than the surface's
>>> - * width then it is clamped to the width of the surface. Same with
>>> - * height.
>>> + * Clamp the rectangles so that they lie within the surface.
>>>   */
>>>
>>>  static void
>>> @@ -1334,17 +1332,14 @@ _eglSetDamageRegionKHRClampRects(_EGLDisplay* disp, _EGLSurface* surf,
>>>     EGLint surf_width = surf->Width;
>>>
>>>     for (i = 0; i < (4 * n_rects); i += 4) {
>>> -      EGLint x, y, rect_width, rect_height;
>>> -      x = rects[i];
>>> -      y = rects[i + 1];
>>> -      rect_width = rects[i + 2];
>>> -      rect_height = rects[i + 3];
>>> -
>>> -      if (rect_width > surf_width - x)
>>> -         rects[i + 2] = surf_width - x;
>>> -
>>> -      if (rect_height > surf_height - y)
>>> -         rects[i + 3] = surf_height - y;
>>> +      EGLint x, y;
>>> +      x = CLAMP(rects[i], 0, surf_width);
>>> +      y = CLAMP(rects[i + 1], 0, surf_height);
>>> +
>>> +      rects[i] = x;
>>> +      rects[i + 1] = y;
>>> +      rects[i + 2] = CLAMP(rects[i + 2], x, surf_width);
>>> +      rects[i + 3] = CLAMP(rects[i + 3], y, surf_height);
>>
>> I believe the previous code for clamping the width/height was correct.
>> Note that this might end up with 0-width/height rects, hopefully the
>> downstream code can handle it.
>>
>
> Thanks for the review.
> The spec says:
>     "Rectangles that lie (partially) outside of the current surface
>     dimensions (as queryable via the EGL_WIDTH and EGL_HEIGHT
>     attributes) will be clamped to the current surface dimensions."
>
> I believe this also includes regions < 0?
> Also, I observed that the rects passed by deqp also include negative
> values. Even in the previous method we could end up with 0 width-height.
> Example: {surf_width, surf_height, X, X}, for any value of X we would
> have clamped it to 0-width/height rect.
> Yes, the downstream should handle these 0 area cases.

Right, so that all has to be handled cleanly. The value in rects is:

    <rects> is a pointer to a list of values describing the rectangles. The list
    should consist of <n_rects> groups of four values, with each group
    representing a single rectangle in surface coordinates in the form {x, y,
    width, height}.

So it's unclear why the lower bound on [2] and [3] are x/y in your
version. If x is -1 and width is 2, what should the outcome be? x = 0
and width = 1, I'd guess?

  -ilia