[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 <k.kahurani(a)gmail.com>

--
  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 <k.kahurani(a)gmail.com>

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 <k.kahurani(a)gmail.com>
---
 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);
-- 
GitLab

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

[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.

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

[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.

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

[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.

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