[PATCH v4 0/1] MR3288: gdiplus: Clip polygons before drawing them

[David Kahurani (＠kahurani)]

Validate user data before passing it to PolyDraw.

The program in the bug requests to draw figures outrageously outside the DC's region after presumably, some uninitialized values happen as a result of a missing font. Native gdiplus seems to handle this gracefully so we probably also should.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41617

Signed-off-by: David Kahurani

--
  v4: gdiplus: Clip polygons before drawing them

https://gitlab.winehq.org/wine/wine/-/merge_requests/3288

[PATCH v4 1/1] gdiplus: Clip polygons before drawing them

[David Kahurani]

From: David Kahurani

This should reduce the load on GDI when an application is trying to
draw to regions outside of selected DC's region

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41617
Signed-off-by: David Kahurani 
---
 dlls/gdiplus/graphics.c | 181 +++++++++++++++++++++++++++++-----------
 1 file changed, 134 insertions(+), 47 deletions(-)

diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c
index 6dc34707bbf..ddac6cbe10e 100644
--- a/dlls/gdiplus/graphics.c
+++ b/dlls/gdiplus/graphics.c
@@ -1920,6 +1920,139 @@ static void shorten_bezier_amt(GpPointF * pt, REAL amt, BOOL rev)
     }
 }

+static GpStatus get_graphics_device_bounds(GpGraphics* graphics, GpRectF* rect)
+{
+    RECT wnd_rect;
+    GpStatus stat=Ok;
+    GpUnit unit;
+
+    if(graphics->hwnd) {
+        if(!GetClientRect(graphics->hwnd, &wnd_rect))
+            return GenericError;
+
+        rect->X = wnd_rect.left;
+        rect->Y = wnd_rect.top;
+        rect->Width = wnd_rect.right - wnd_rect.left;
+        rect->Height = wnd_rect.bottom - wnd_rect.top;
+    }else if (graphics->image){
+        stat = GdipGetImageBounds(graphics->image, rect, &unit);
+        if (stat == Ok && unit != UnitPixel)
+            FIXME("need to convert from unit %i\n", unit);
+    }else if (GetObjectType(graphics->hdc) == OBJ_MEMDC){
+        HBITMAP hbmp;
+        BITMAP bmp;
+
+        rect->X = 0;
+        rect->Y = 0;
+
+        hbmp = GetCurrentObject(graphics->hdc, OBJ_BITMAP);
+        if (hbmp && GetObjectW(hbmp, sizeof(bmp), &bmp))
+        {
+            rect->Width = bmp.bmWidth;
+            rect->Height = bmp.bmHeight;
+        }
+        else
+        {
+            /* FIXME: ??? */
+            rect->Width = 1;
+            rect->Height = 1;
+        }
+    }else{
+        rect->X = 0;
+        rect->Y = 0;
+        rect->Width = GetDeviceCaps(graphics->hdc, HORZRES);
+        rect->Height = GetDeviceCaps(graphics->hdc, VERTRES);
+    }
+
+    return stat;
+}
+
+static void poly_clip(GpGraphics *graphics, POINT *pti, BYTE *tp, int count)
+{
+    RectF rect;
+    BYTE *tpcopy = heap_alloc_zero(count * sizeof(BYTE));
+
+    memcpy(tpcopy, tp, count);
+    get_graphics_device_bounds(graphics, &rect);
+
+    /* Ignore data that is not meant for drawing */
+    if (rect.Width == 1 && rect.Height == 1)
+        return;
+
+    for (int i = 0; i < count; i++)
+    {
+         switch(tp[i] &~ PT_CLOSEFIGURE)
+         {
+         case PT_BEZIERTO:
+         {
+             int j;
+
+             for (j = 0; j < 3; j++)
+             {
+                  REAL x = pti[i + j].x;
+                  REAL y = pti[i + j].y;
+
+                  /* At least part of the rectangle encompassing the bezier lies within the DC's region */
+                  if (rect.X <= x && rect.Y <= y && x <= rect.X + rect.Width && y <= rect.Y + rect.Height)
+                      break;
+             }
+
+             if (j == 3)
+             {
+                 for(j = 0; j < 3; j++)
+                     tp[i + j] = PT_MOVETO;
+             }
+
+             i += 2;
+             break;
+         }
+         case PT_LINETO:
+         {
+             GpPath *path;
+             GpPen *pen;
+             GpRegion *line_region;
+             BOOL empty;
+
+             GdipCreatePath(FillModeAlternate, &path);
+
+             if (i == 0)
+                 GdipAddPathLine(path, 0, 0, pti[i].x, pti[i].y);
+             else
+                 GdipAddPathLine(path, pti[i - 1].x, pti[i - 1].y, pti[i].x, pti[i].y);
+
+             GdipCreatePen1(0xffffffff, 10.0, UnitPixel, &pen);
+             GdipWidenPath(path, pen, NULL, FlatnessDefault);
+             GdipCreateRegionPath(path, &line_region);
+             GdipCombineRegionRect(line_region, &rect, CombineModeIntersect);
+             GdipIsEmptyRegion(line_region, graphics, &empty);
+
+             if (!empty)
+             {
+                 if (i != 0)
+                 {
+                     tp[i - 1] = tpcopy[i - 1];
+                     tp[i] = tpcopy[i];
+                 }
+             }
+             else
+                  tp[i] = PT_MOVETO;
+
+             GdipDeleteRegion(line_region);
+             GdipDeletePen(pen);
+             GdipDeletePath(path);
+             break;
+         }
+         case PT_MOVETO:
+             break;
+
+         default:
+             ERR("Bad point type \n");
+         }
+    }
+
+    free(tpcopy);
+}
+
 /* Draws a combination of bezier curves and lines between points. */
 static GpStatus draw_poly(GpGraphics *graphics, GpPen *pen, GDIPCONST GpPointF * pt,
     GDIPCONST BYTE * types, INT count, BOOL caps)
@@ -2036,6 +2169,7 @@ static GpStatus draw_poly(GpGraphics *graphics, GpPen *pen, GDIPCONST GpPointF *
         tp[i] = convert_path_point_type(types[i]);
     }

+    poly_clip(graphics, pti, tp, count);
     PolyDraw(graphics->hdc, pti, tp, count);

status = Ok;
@@ -2153,53 +2287,6 @@ static GpStatus restore_container(GpGraphics* graphics,
     return Ok;
 }

-static GpStatus get_graphics_device_bounds(GpGraphics* graphics, GpRectF* rect)
-{
-    RECT wnd_rect;
-    GpStatus stat=Ok;
-    GpUnit unit;
-
-    if(graphics->hwnd) {
-        if(!GetClientRect(graphics->hwnd, &wnd_rect))
-            return GenericError;
-
-        rect->X = wnd_rect.left;
-        rect->Y = wnd_rect.top;
-        rect->Width = wnd_rect.right - wnd_rect.left;
-        rect->Height = wnd_rect.bottom - wnd_rect.top;
-    }else if (graphics->image){
-        stat = GdipGetImageBounds(graphics->image, rect, &unit);
-        if (stat == Ok && unit != UnitPixel)
-            FIXME("need to convert from unit %i\n", unit);
-    }else if (GetObjectType(graphics->hdc) == OBJ_MEMDC){
-        HBITMAP hbmp;
-        BITMAP bmp;
-
-        rect->X = 0;
-        rect->Y = 0;
-
-        hbmp = GetCurrentObject(graphics->hdc, OBJ_BITMAP);
-        if (hbmp && GetObjectW(hbmp, sizeof(bmp), &bmp))
-        {
-            rect->Width = bmp.bmWidth;
-            rect->Height = bmp.bmHeight;
-        }
-        else
-        {
-            /* FIXME: ??? */
-            rect->Width = 1;
-            rect->Height = 1;
-        }
-    }else{
-        rect->X = 0;
-        rect->Y = 0;
-        rect->Width = GetDeviceCaps(graphics->hdc, HORZRES);
-        rect->Height = GetDeviceCaps(graphics->hdc, VERTRES);
-    }
-
-    return stat;
-}
-
 static GpStatus get_graphics_bounds(GpGraphics* graphics, GpRectF* rect)
 {
     GpStatus stat = get_graphics_device_bounds(graphics, rect);

[Huw Davies (＠huw)]

On Fri Jul 14 08:57:50 2023 +0000, Esme Povirk wrote:
> @huw I wanted you to be in this discussion because I'm not as familiar
> with PolyDraw input (though at the moment the transformation of those
> inputs looks flawed to me so you can probably hold off on that), and I'd
> like to know if this is something that would make more sense in gdi32.

If there's a good optimization that we could use in gdi32 then we'd certainly prefer it went there instead.

[Esme Povirk (＠madewokherd)]

On Sat Jul 15 20:07:15 2023 +0000, David Kahurani wrote:
> I might not be qualified to comment on this and are not sure exactly
> what is the question. I am going to assume the question is whether the
> changes that this code is making to the input improve things for gdi32.
> My answer is yes. The code essentially replaces invalid points with the
> PT_MOVETO type. PolyDraw(at least the wine version) ignores PT_MOVETO
> type points or uses them as a starting point to draw.
> When we generate consequent PT_MOVETO type points, all will get ignored
> except the last which will be used as the starting point for a valid
> line segment.
> To sum it up, PT_MOVETO type of points are ultra cheap hence why I used
> them here instead of complicating everything while trying to come up
> with a new set of updated points.

The question is whether gdi32 can identify these points and skip them. If so, the optimization probably makes more sense in gdi32 instead of gdiplus.

[Esme Povirk (＠madewokherd)]

On Wed Jul 12 20:26:34 2023 +0000, David Kahurani wrote:
> I updated this to use regions but I believe that line
> thickness(artificially created) may lead to some false positives....
> around the edges and corners of the rectangle but it looks to me like
> these are unlikely to cause any serious issues.

False negatives (lines that do not need to be drawn but are) are acceptable. False positives are not. The user may be slowly scrolling a view, with only the edge being rendered, in which case inaccuracies near the edge would lead to noticeable artifacts.

GDI+ path and region functions are too expensive to be worth using in an optimization like this.

To prevent false positives, I think you would have to:
 * Calculate a rectangle for graphics bounds with an expansion to account for line width.
 * Calculate the bounding box of points in each line or bezier segment.
 * Check for an intersection between the two rectangles.