1868067 - aa-stroke fails to render half-arcs

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Kaiido]

Attachment: test-case.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0

Steps to reproduce:

Originally reported at https://stackoverflow.com/questions/77584584/line-endings-in-js-canvas-arc-function-bug-in-firefox
Online test case (jsffidle).

Stroke an half arc using a line-width equal to the arc's diameter.

Actual results:

The stroke does overlap in the center of the arc and fails to render the extremities of the arc (see the various attached screenshots, and particularly xor.png).

Expected results:

A clean half circle should have been drawn.

This only happens when using GPU rendering and is caused by this CL. At that time, the bug was a bit different (see attachment initial-bug.png). Since this CL it is in the current state.

The whole strategy of https://bugzilla.mozilla.org/show_bug.cgi?id=1837068 seems very fragile. Even when using smaller line-width, the composition over the antialias will result in visible holes (see holes.png, where I added the red circles in post prod and holes.html).
I even had a temporary aliasing bug when rendering the stroke with some alpha where I could see each lines of the stroke converging to the center of the arc. I unfortunately failed to take a screenshot of that and couldn't repro since then, even during bisect.

Comment 1

[Kaiido]

Attachment: current.png

Comment 2

[Kaiido]

Attachment: xor.png

Comment 3

[Kaiido]

Attachment: initial-bug.png

Comment 4

[Kaiido]

Attachment: holes.html

Comment 5

[Kaiido]

Attachment: holes.png

Comment 6

[Mayank Bansal]

The arc rendering issue is probably a dupe of bug 1867774. The hole rendering issue was first seen in https://bugzilla.mozilla.org/attachment.cgi?id=9341757

Comment 7

[Kaiido]

Oh, indeed, bug 1867774 is about the same issue, sorry I missed it.
From which bug comes the hole screenshot though?

Comment 8

[Timothy Nikkel (:tnikkel)]

As expected, caused by bug 1834079.

Comment 9

[Jim Mathies [:jimm]]

Not sure what severity we should give. Looks fairly serious, and is a recent regression related to some speedometer 3 improvements in rendering path selection.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Windows is unaffected (because of d2d?) but other platforms are affected. jrmuizel has repro'd.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Can reproduce this in a standalone test case

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: Testcase from bug 1867774

Jeff, any plans on how to fix this?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Yes, I want to stroke/offset curves during flattening instead of flattening first and then stroking. This will let us have proper tangents and the start and end of the curve instead of the tangents of the flattened curve. The thing that blocks this from being easy to implement is the possibility of self intersection for a particular segment.

If we wanted to mitigate this problem temporarily we could just adjust the flattening tolerance in proportion to the line width.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP to stroke curves with trapezoids instead of rectangles

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1868067. Rasterize curves directly with trapezoids instead of flattening.

This fixes drawing half arcs, reduces the triangle count when flattening
because we don't have joins and thus improves performance.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc3f01a435d2
Rasterize curves directly with trapezoids instead of flattening. r=lsalzman

Comment 18

[agoloman]

Backed out for causing mochitest failures @test_toDataURL_parameters_png.html.

• Backout link
• Push with failures
• Failure Log

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/769b2c148e19
Rasterize curves directly with trapezoids instead of flattening. r=lsalzman

Comment 20

[Cristina Horotan [:chorotan]]

Backed out for causing build bustages

Backout link

Push with failures

Failure log

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db48fd7bd3ec
Rasterize curves directly with trapezoids instead of flattening. r=lsalzman

Comment 22

[amarc]

https://hg.mozilla.org/mozilla-central/rev/db48fd7bd3ec

Comment 25

[Norisz Fay [:noriszfay]]

Backed out as requested by dev for causing Bug 1937287

Backout link

Comment 26

[Serban Stanca [:SerbanS]]

Backout merged in central: https://hg.mozilla.org/mozilla-central/rev/a4854d53bebf4f7c63929ea009fbd634db1e590d.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1da3b580ac68
Rasterize curves directly with trapezoids instead of flattening. r=lsalzman

Comment 28

[agoloman]

Backed out for causing build bustages.

• Backout link
• Push with failures
• Failure Log

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/398734cfaf84
Rasterize curves directly with trapezoids instead of flattening. r=lsalzman

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/398734cfaf84

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jrmuizel, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regressions caused by this fix.
• If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

We've been shipping this long enough on other platforms and the implementation has enough risk that I'd prefer to let this bake.