[Bug 59718] New: d2d1: Three correctness issues in arc Bezier-approximation code (d2d_arc_to_bezier, d2d_figure_add_arc, d2d_arc_transform)
http://bugs.winehq.org/show_bug.cgi?id=59718 Bug ID: 59718 Summary: d2d1: Three correctness issues in arc Bezier-approximation code (d2d_arc_to_bezier, d2d_figure_add_arc, d2d_arc_transform) Product: Wine Version: 11.8 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: d2d Assignee: wine-bugs@list.winehq.org Reporter: nen24t@gmail.com Distribution: --- Code review of the arc Bezier-approximation code introduced in commits - `5d093518d71` ("d2d1: Implement arc approximation with Bezier segments.") — adds `d2d_arc_to_bezier()` and `d2d_figure_add_arc()` - `c34a09e285c` ("d2d1: Create a path internally for the geometry group.") — adds `d2d_arc_transform()` revealed three correctness issues. All three are present unchanged in `wine-11.8`. They are independent of each other but related (all three are in the arc-handling path), so I'm filing them as a single bug for convenience. Happy to split into three if preferred. ### Issue 1 — Uninitialized `points[0]` used as line endpoint in degenerate-radius case In `dlls/d2d1/geometry.c`: ```c static int d2d_arc_to_bezier(const D2D_POINT_2F *start_point, const D2D1_ARC_SEGMENT *arc, D2D_POINT_2F *points) { ... if (!d2d_arc_check_radius(rHalfChord2, fuzz2, &radius.x) || !d2d_arc_check_radius(rHalfChord2, fuzz2, &radius.y)) { return 0; /* points[] never written */ } ... } static bool d2d_figure_add_arc(struct d2d_figure *figure, const D2D1_ARC_SEGMENT *arc) { size_t last = figure->vertex_count - 1; D2D_POINT_2F points[12]; int count = 0; count = d2d_arc_to_bezier(&figure->vertices[last], arc, points); if (count > 0) return d2d_figure_add_beziers(figure, (D2D1_BEZIER_SEGMENT *)points, count); else if (count == 0) return d2d_figure_add_lines(figure, points, 1); /* points[0] is uninitialized */ return true; } ``` When `d2d_arc_to_bezier()` returns 0 (radius check failed), `points[]` is left uninitialized but the caller passes `points[0]` to `d2d_figure_add_lines()` as a line endpoint. Result: garbage line endpoint, undefined visual behavior. Reasonable fix would be to use `arc->point` (the arc's specified end point) as the line endpoint in the degenerate case — but the function signature doesn't currently expose `arc` to the caller of `d2d_figure_add_lines()`, so either `d2d_arc_to_bezier()` should write `arc->point` into `points[0]` before returning 0, or `d2d_figure_add_arc()` should pass `&arc->point` directly. --- ### Issue 2 — Typo in arc center transform: `m._12 * center.x` should be `m._12 * center.y` In `d2d_arc_to_bezier()` after computing the arc center: ```c m._31 = 0.5f * (arc->point.x + start_point->x); m._32 = 0.5f * (arc->point.y + start_point->y); if (!zero_center) { m._31 += (m._11 * center.x + m._12 * center.x); /* second term should be center.y */ m._32 += (m._21 * center.x + m._22 * center.y); /* this line is correct */ } ``` The first line uses `center.x` twice. The intended computation is the linear matrix-vector product `M * center`: ``` (_31, _32) += (M11 * cx + M12 * cy, M21 * cx + M22 * cy) ``` so the first line should be: ```c m._31 += (m._11 * center.x + m._12 * center.y); ``` Effect is invisible when `m._12 == 0` (i.e. `arc->rotationAngle == 0`), which is the most common case in UI rendering. Becomes visible for rotated arcs / rotated ellipse-segments, where the arc center will be displaced along the rotation axis. --- ### Issue 3 — `d2d_arc_transform()` contaminates radius vectors with translation component ```c static inline void d2d_arc_transform(D2D1_ARC_SEGMENT *arc, const D2D1_MATRIX_3X2_F *transform) { D2D_POINT_2F point; d2d_point_transform(&arc->point, transform, arc->point.x, arc->point.y); d2d_point_transform(&point, transform, arc->size.width, 0.0f); arc->size.width = d2d_point_length(&point); d2d_point_transform(&point, transform, 0.0f, arc->size.height); arc->size.height = d2d_point_length(&point); } ``` `d2d_point_transform()` applies the full affine transform including the `_31`/`_32` translation. For the arc endpoint (`arc->point`) that is correct. For the radius vectors `(width, 0)` and `(0, height)` it is wrong: those represent direction/length, not absolute positions, and should be transformed with the linear part only. Concretely, with a translation `(tx, ty)`: ``` transform of (width, 0) = (width * _11 + tx, width * _12 + ty) length(...) = sqrt((width*_11 + tx)^2 + (width*_12 + ty)^2) ``` The correct value is `sqrt((width*_11)^2 + (width*_12)^2) = width * sqrt(_11^2 + _12^2)`, the magnitude of the linear part only. Fix: compute the radius vectors relative to the transformed origin, e.g.: ```c static inline void d2d_arc_transform(D2D1_ARC_SEGMENT *arc, const D2D1_MATRIX_3X2_F *transform) { D2D_POINT_2F origin, w_axis, h_axis; d2d_point_transform(&origin, transform, 0.0f, 0.0f); d2d_point_transform(&w_axis, transform, arc->size.width, 0.0f); d2d_point_transform(&h_axis, transform, 0.0f, arc->size.height); d2d_point_transform(&arc->point, transform, arc->point.x, arc->point.y); arc->size.width = sqrtf((w_axis.x - origin.x) * (w_axis.x - origin.x) + (w_axis.y - origin.y) * (w_axis.y - origin.y)); arc->size.height = sqrtf((h_axis.x - origin.x) * (h_axis.x - origin.x) + (h_axis.y - origin.y) * (h_axis.y - origin.y)); } ``` Visible only for transformed paths (`ID2D1TransformedGeometry`, geometry-group internal stream) where the transform contains a non-zero translation — common for plugin host UIs with offset rendering layers. --- ## How to reproduce Each issue requires a different setup: - **Issue 1**: Build a path geometry with `AddArc()` whose start/end points are far apart but `arc->size.width` or `arc->size.height` is below the implementation's fuzz threshold (very thin arc). Render — random line endpoints will appear. - **Issue 2**: Use `AddArc()` with `rotationAngle != 0` (e.g. 30°) with a non-degenerate arc. Compare against native Direct2D in Windows — the arc center will be slightly displaced. - **Issue 3**: Wrap a path containing an arc in `ID2D1TransformedGeometry` with a transform that has both a non-trivial linear part *and* a translation. Stream the transformed geometry — the arc radius will be wrong. --- ## Affected versions `wine-11.0` through `wine-11.8`. All three issues are present unchanged across the range. ## Reporter context These came up while reviewing the 11.x → 12.0 rebase plan for an out-of-tree D2D1 patch series targeted at JUCE 8 / VSTGUI plugin compatibility. The plan adopts Sivov's Stream-Path + Bezier-Arc architecture as the upstream-compatible foundation, so we wanted to flag these issues before settling on it. References: - Fork: https://github.com/giang17/wine - Active branch: https://github.com/giang17/wine/tree/d2d1-dcomp-11.0 - Discussion thread: https://github.com/robbert-vdh/yabridge/issues/413 - Related earlier bug (DWrite text rendering): https://bugs.winehq.org/show_bug.cgi?id=59365 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=59718 --- Comment #1 from Nikolay Sivov <bunglehead@gmail.com> --- Thank you for the report. Do you plan to work on fixes for those and submit them to wine? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=59718 --- Comment #2 from gng <nen24t@gmail.com> --- Thanks Nikolay — sure, happy to take this on. All three fixes are now on wine-devel: - d2d1: Fix arc center translation to use center.y in _31 update. - d2d1: Fix uninitialized points[0] in degenerate-radius arc fallback. - d2d1: Strip translation from transform when scaling arc radii. No tests included for any of them — the bugs are straightforward enough to verify by inspection. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=59718 --- Comment #3 from Nikolay Sivov <bunglehead@gmail.com> --- Please submit them with a single merge request, it's described here https://gitlab.winehq.org/wine/wine/-/wikis/Submitting-Patches. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=59718 --- Comment #4 from gng <nen24t@gmail.com> --- Thanks — will do, single MR with the three commits as soon as my GitLab access request is approved. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=59718 Nikolay Sivov <bunglehead@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED --- Comment #5 from Nikolay Sivov <bunglehead@gmail.com> --- This is merged. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=59718 --- Comment #6 from Giang Nguyen <nen24t@gmail.com> --- Thanks! Also cherry-picked into giang17/wine#d2d1-dcomp-11.0 for fork users on Wine 11.0 base. Verified with Trinity + Serum 2. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla