[PATCH 0/3] MR10813: d2d1: Three arc-handling bug fixes (Wine-Bug 59718)
This series fixes three independent correctness issues in the d2d1 arc Bezier-approximation code, all reported in Wine-Bug 59718: 1. d2d_arc_to_bezier(): degenerate-radius fallback uses uninitialized points[0] as the line endpoint (introduced in 5d093518d). 2. d2d_arc_to_bezier(): typo in arc center translation, m._12 * center.x where center.y is intended (introduced in 5d093518d). 3. d2d_arc_transform(): radii are direction vectors and should be transformed with the linear part of the matrix only; the current code includes the translation, yielding wrong lengths whenever the transform has a non-zero translation (introduced in c34a09e285). The three commits are independent and can be applied in any order. No tests included — all three are clearly verifiable by inspection, but happy to add specific test cases for any of them if useful. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59718 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10813
From: Giang Nguyen <nen24t@gmail.com> When d2d_arc_to_bezier() rejects an arc because both radii are below the fuzz threshold, it returns 0 to signal "treat as a straight line". The caller d2d_figure_add_arc() then passes points[0] to d2d_figure_add_lines() as the line endpoint: count = d2d_arc_to_bezier(&figure->vertices[last], arc, points); if (count > 0) return d2d_figure_add_beziers(figure, ...); else if (count == 0) return d2d_figure_add_lines(figure, points, 1); But points[0] is never written on the radius-rejection path, so add_lines() consumes uninitialized stack memory as the endpoint. Result: random line target, undefined visual behavior for any path that contains a degenerate-radius arc. Fix is to write the arc's specified end point into points[0] before returning 0, which matches the natural fallback semantics ("draw a line from the previous vertex to the arc's endpoint"). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59718 Signed-off-by: Giang Nguyen <nen24t@gmail.com> --- dlls/d2d1/geometry.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/d2d1/geometry.c b/dlls/d2d1/geometry.c index 68de4c2193c..51d587673dd 100644 --- a/dlls/d2d1/geometry.c +++ b/dlls/d2d1/geometry.c @@ -1247,6 +1247,7 @@ static int d2d_arc_to_bezier(const D2D_POINT_2F *start_point, const D2D1_ARC_SEG if (!d2d_arc_check_radius(rHalfChord2, fuzz2, &radius.x) || !d2d_arc_check_radius(rHalfChord2, fuzz2, &radius.y)) { + points[0] = arc->point; return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10813
From: Giang Nguyen <nen24t@gmail.com> In d2d_arc_to_bezier(), when computing the affine transform that maps the canonical unit-arc parameter space to the actual arc, the translation contribution from the arc center is meant to be the linear matrix-vector product M * center, i.e.: (_31, _32) += (M11*cx + M12*cy, M21*cx + M22*cy) The first line of the update used center.x in both terms, which yields M11*cx + M12*cx instead of M11*cx + M12*cy. Effect is invisible when M12 == 0 (i.e. arc->rotationAngle == 0), which is the common case for unrotated arcs in UI rendering. Becomes visible for arcs with a non-zero rotationAngle, where the arc center ends up displaced along the rotation axis. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59718 Signed-off-by: Giang Nguyen <nen24t@gmail.com> --- dlls/d2d1/geometry.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/d2d1/geometry.c b/dlls/d2d1/geometry.c index 51d587673dd..a793e298c2f 100644 --- a/dlls/d2d1/geometry.c +++ b/dlls/d2d1/geometry.c @@ -1312,7 +1312,7 @@ static int d2d_arc_to_bezier(const D2D_POINT_2F *start_point, const D2D1_ARC_SEG m._32 = 0.5f * (arc->point.y + start_point->y); if (!zero_center) { - m._31 += (m._11 * center.x + m._12 * center.x); + m._31 += (m._11 * center.x + m._12 * center.y); m._32 += (m._21 * center.x + m._22 * center.y); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10813
From: Giang Nguyen <nen24t@gmail.com> d2d_arc_transform() applies the same affine matrix to the arc's end point and to its two radius vectors (width, 0) and (0, height). That is correct for the end point (a position) but wrong for the radii (direction vectors): when the matrix includes a non-zero translation (_31, _32), the transformed radius vector becomes (width * _11 + _31, width * _12 + _32) and the resulting d2d_point_length() returns sqrt((width*_11 + _31)^2 + (width*_12 + _32)^2) instead of the intended sqrt((width*_11)^2 + (width*_12)^2) = width * sqrt(_11^2 + _12^2) i.e. the magnitude of the linear part only. Visible whenever a path that contains an arc is wrapped in a transformed geometry whose transform has both a non-trivial linear part and a translation — common for plugin host UIs with offset rendering. Fix: build a local copy of the matrix with _31 = _32 = 0 and use it for the radius transforms. Keep the original transform for the end point. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59718 Signed-off-by: Giang Nguyen <nen24t@gmail.com> --- dlls/d2d1/geometry.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/dlls/d2d1/geometry.c b/dlls/d2d1/geometry.c index a793e298c2f..ee73c128ce2 100644 --- a/dlls/d2d1/geometry.c +++ b/dlls/d2d1/geometry.c @@ -4480,12 +4480,19 @@ static HRESULT STDMETHODCALLTYPE d2d_path_geometry_Open(ID2D1PathGeometry1 *ifac static inline void d2d_arc_transform(D2D1_ARC_SEGMENT *arc, const D2D1_MATRIX_3X2_F *transform) { + D2D1_MATRIX_3X2_F linear = *transform; D2D_POINT_2F point; + /* Radii are direction vectors, so they should be transformed with the + * linear part of the matrix only. Including the translation would add + * (_31, _32) to (width, 0) and (0, height) and yield incorrect lengths. */ + linear._31 = 0.0f; + linear._32 = 0.0f; + d2d_point_transform(&arc->point, transform, arc->point.x, arc->point.y); - d2d_point_transform(&point, transform, arc->size.width, 0.0f); + d2d_point_transform(&point, &linear, arc->size.width, 0.0f); arc->size.width = d2d_point_length(&point); - d2d_point_transform(&point, transform, 0.0f, arc->size.height); + d2d_point_transform(&point, &linear, 0.0f, arc->size.height); arc->size.height = d2d_point_length(&point); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10813
Was AI used to generate any of this code? Some of the patches on your github branch linked in that bug report mention Claude - our [clean room guidlines](https://gitlab.winehq.org/wine/wine/-/wikis/Clean-Room-Guidelines) prohibit LLM generated code. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10813#note_138738
Hi Connor — yes, I used Claude for the code review and patch formulation on these. The clean-room policy update was added two weeks ago and I missed that LLM-generated code is now explicitly prohibited; I should have flagged that proactively. Sorry for the noise. Closing this MR. The bugs themselves are documented in detail in bug 59718, and the fixes are trivial enough that any human reviewer working clean-room can produce them — happy for that to be Nikolay's, yours, or anyone else's. I'll keep my own fork separate from upstream-submission going forward. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10813#note_138741
This merge request was closed by Giang Nguyen. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10813
participants (3)
-
Connor McAdams (＠cmcadams) -
Giang Nguyen
-
Giang Nguyen (＠giang17)