1544413 - Broken SVG with large coordinates (with Skia and WebRender backends, though not with Cairo)

Status: ASSIGNED

(Core :: Graphics: WebRender, defect, P2)

Description

[Florian Rhiem]

Attachment: Sample file

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

Steps to reproduce:

I am viewing the attached SVG file.

Actual results:

The polyline stops being rendered before reaching the boundary of the clipping rect.

Expected results:

The polyline should reach the boundary of the clipping rect.

Comment 1

[violet.bugreport]

Attachment: reduced-testcase.html

Comment 2

[violet.bugreport]

It's not related to clipping. Our coordinate in layout system is internally represented by int (with 60x factor), for large number it will be truncated. This bug is unlikely to be fixed any time soon...

Comment 3

[Daniel Holbert [:dholbert]]

There's an existing old bug filed on this, too, though I can't find it at the moment.

Comment 10

[Mathew Hodson]

According to the most recently added duplicate, this was regressed by bug 767734.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to violet.bugreport from comment #2)

It's not related to clipping. Our coordinate in layout system is internally represented by int (with 60x factor), for large number it will be truncated. This bug is unlikely to be fixed any time soon...

Correction on this -- this statement^^ is true for CSS layout (of block/flexbox/floats/etc), but for the coordinate space inside of an SVG element, I believe we use float, and then that gets translated into whatever our underlying graphics layer uses (which is also some form of float).

(There's still the potential for loss of precision at large values -- i.e. LARGE_NUMBER and LARGE_NUMBER+1 may be indistinguishable, for some values of LARGE_NUMBER -- but we won't just hit the maximum value and wrap around like we would with int.)

(In reply to Mathew Hodson from comment #10)

According to the most recently added duplicate, this was regressed by bug 767734.

This does not seem to be true for the original testcase here (based on trying mozregression before vs. after bug 767734's patch), so perhaps they're not dupes after all...

Comment 12

[Daniel Holbert [:dholbert]]

A few observations from looking at the attached testcases in Nightly and mozregression:
OBSERVATION #1:

• reduced-testcase.html actually works just fine in current Nightly (and Nightly from the day that this bug was filed), if I turn off WebRender.
• However, the original testcase is still broken regardless of whether WebRender is off.
• So: reduced-testcase.html seems to have narrowed in on a different WebRender-specific bug. (Or perhaps it's a version of the same issue but it's possible to trigger it with a simpler testcase when WebRender is on.)

OBSERVATION #2:
Using the original testcase in mozregression, it looks like we actually render the testcase correctly as recently as August 2016, on my linux machine.
Nightly 2016-08-25 produces EXPECTED RESULTS (vertical line extends all the way down), whereas Nightly 2016-08-26 produces an extra-severe broken result (completely blank SVG). This seems to have been a regression from https://hg.mozilla.org/mozilla-central/rev/d0910ce4cff8a1994e4d1d975ef7077f57f05668 which was a commit to switch us from Cairo to Skia as our backend on linux. (I confirmed that that was the culprit by trying the "broken" build with the pref reset to use cairo, like so:

mozregression --launch 2016-08-26  -a https://bugzilla.mozilla.org/attachment.cgi?id=9058252  --pref "gfx.content.azure.backends:'cairo'"

...and this gave me EXPECTED RESULTS.

I'm not sure yet what caused us to go from blank-SVG-area to line-partially-drawn-in-SVG-area, but I'm willing to bet it was another graphics backend change (perhaps a Skia upgrade).

So, this leads me to believe this is a bug in our graphics backends (Skia and WebRender, at least), rather than a bug in our SVG-specific code.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #12)

I'm not sure yet what caused us to go from blank-SVG-area to line-partially-drawn-in-SVG-area, but I'm willing to bet it was another graphics backend change (perhaps a Skia upgrade).

Pushlog for this change (the partial restoration of functionality, bringing us to the current behavior, with a partial-line drawn in the SVG area starting at Nightly 2017-04-07):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c68d659c2b715f811708f043a1e7169d77be2ba&tochange=10ea10d9993c9701e5525928257a589dea2c05d8

I'm not sure what changed in there to get things less-broken; perhaps it was one of the SVG transform-related commits.

In any case, though: it looks like we can still force the cairo drawing backend even in current Nightly builds -- and with that forced, I get EXPECTED RESULTS (full vertical line) from the original testcase. Here's the command I'm using (notice 2020-06-01, i.e. yesterday's Nightly):

mozregression --launch 2020-06-01  -a https://bugzilla.mozilla.org/attachment.cgi?id=9058252  --pref "gfx.content.azure.backends:'cairo'"

So, this strongly suggests that the testcases here are symptoms of a precision bug in our Skia and WebRender graphics backends (likely due to 32-bit floats losing precision at large values, or something along those lines.)

--> Reclassifying as Graphics

Comment 14

[Jessie [:jbonisteel] pls NI]

Can we see the original test case for this? I believe it involved powerpoint online?

Comment 15

[Florian Rhiem]

The test case I posted was the result of trying to find the cause of an issue reported to the GR framework, which can output SVG files: https://github.com/jheinen/GR.jl/issues/210

I reduced the SVG file created by GR, then simplified it further for this bug report here.

Comment 16

[Daniel Holbert [:dholbert]]

(I don't have the powerpoint testcase, though I've heard talk of a PowerPoint testcase that runs afoul of this & martin may know more. I've just been working off of the attachments here -- Florian's "Sample file" and violet's "reduced-testcase.html")

Comment 17

[Jessie [:jbonisteel] pls NI]

Yeah knowing more about this PowerPoint scenario will be useful.

Comment 18

[arista]

I logged this bug with a test file generated from PowerPoint
https://bugzilla.mozilla.org/show_bug.cgi?id=1554015

Comment 19

[Martin Balfanz [:mbalfanz]]

Jessie, I'll get back to the reporter and see if I can get the original case.

@arista: Thanks for pointing to the other bug! I really appreciate this additional test case.
I tried to reproduce a similar SVG, but in the web version of PowerPoint everything seemed to work fine (even for very large shapes). I also tried the Desktop/Mac version, but it seems like neither the web nor the Mac version allow me to export SVGs. Could you please describe how you created the shape/SVG and a typical use case? I try to understand how a user would encounter this problem.

Comment 20

[arista]

I've been trying to find repro steps for the last 2 days and I can't.
The original file I got from PowerPoint Online in SlideShow, but nowadays it uses WebGL in SlideShow.
However this can also reproduce in the editing view, and I've been unable to get it to, or to find the bug with the steps to do it.

I had ran into this a few weeks ago, but for now I can't

Comment 21

[Martin Balfanz [:mbalfanz]]

Thanks for trying again, arista. That matches my experience as well. I will investigate further

Comment 23

[Daniel Holbert [:dholbert]]

https://github.com/webcompat/web-bugs/issues/101970 seems to be another instance of this, with this arc command producing a giant black artifact in Firefox vs. a short line segment in other browsers:
a56270558080367.86,56270558080367.86,0,0,1-1.2,1.4

Possibly-reduced testcase for that (which just renders like a nearly-square "chat bubble" in Chrome, but as a triangle with a slash extending off the bottom right in Firefox):

data:text/html,<svg><path fill="black" d="M20 20 h20 v20 h-20 a56270558080367.86,56270558080367.86,0,0,1-1.2,1.4z">

If I reduce the values to be less-massive like so, then Firefox matches Chrome:

data:text/html,<svg><path fill="black" d="M20 20 h20 v20 h-20 a999999,999999,0,0,1-1.2,1.4z">

If we can identify the particular spot where we're (presumably) overflowing or tripping some threshold, then perhaps we could mitigate some of the badness here by adding some clamping at that point...

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #23)

https://github.com/webcompat/web-bugs/issues/101970 seems to be another instance of this

dshin used mozregression and got this regression range for this particular webcompat issue's testcase (and I confirmed that this regression range is correct for my reduced data-URI testcase in comment 23):
Last good revision: 19fd3388c372 (2013-10-24)
First bad revision: 2f2a45f04e7c (2013-10-25)
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=19fd3388c372&tochange=2f2a45f04e7c

That pushlog is much older than the other ones here (in e.g. comment 12 and comment 13); but it has some commits that make us start using Moz2D for path drawing, which I think (?) may have switched graphics-backends under the hood, or possibly introduced some new float conversions where previously we had double, or something similar.

So: perhaps a distinct earlier regression, but closely related to the other issues discussed here.

Comment 25

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

The severity field for this bug is relatively low, S3. However, the bug has 7 duplicates.
:bhood, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 26

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

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.

Comment 27

[Bob Hood [:bhood]]

I see the reported result with the attached SVG file using sw-wr (which is my default mode), so this issue remains persistent even if the OP does not. I'm dropping it into the WR component for some eyes-on, but leaving the pr/sv settings.

Comment 28

[Bob Hood [:bhood]]

2025 WebCompat: Graphics::WebRender

Comment 29

[Glenn Watson [:gw]]

Consider decreasing the webcompat priority of this because it is a pathological case (only occurs on specific content that contains huge coordinates in SVG space, which is typically not necessary) and likely affects a very small number of users and pages.

Comment 30

[Glenn Watson [:gw]]

I spent some time investigating this today, I believe I've identified roughly where the problem lies (or at least, some interesting differences compared to how chromium rasterizes this test case). Further work on it is going to need someone who knows the Gecko SVG and Skia path rendering integration code better than me.

I was using a reduced test case linked from comment 23 above - the test case is https://github.com/webcompat/web-bugs/issues/101970#issuecomment-1090535565.

The troublesome part of the test is the extremely large coords in the arc command near the end of the test:

a56270558080367.86,56270558080367.86,0,0,1-1.2,1.4

In chromium, this ends up in the arcTo function in [1] (well, that's Firefox's copy of Skia, it may be slightly different from what is in upstream chromium that I was debugging). The code then hits this condition [2], which references a Skia bug and math going "wonky", followed by conversion to a lineTo and exiting the function.

In gecko, when the path is being processed, we end up in [3], which converts the Arc command to one or more BezierTo operations. This seems to be where things go wrong, with the conversion producing incorrect bezier curves that produce the artifact. Commenting out the BezierTo call in this condition makes the artifact disappear.

Questions:

• Is it possible to fix / handle this case in SVGArcConverter so we avoid this artifact without breaking other content?
• Should we skip the arc -> bezier conversion completely and directly call the arcTo methods inside Skia, which appears to handle this edge case?

[1] https://searchfox.org/firefox-main/source/gfx/skia/skia/src/core/SkPath.cpp#1249
[2] https://searchfox.org/firefox-main/source/gfx/skia/skia/src/core/SkPath.cpp#1323
[3] https://searchfox.org/firefox-main/source/dom/svg/SVGPathData.cpp#309

Comment 31

[Robert Longson [:longsonr]]

The SVG spec strongly encourages converting arcs to beziers in various places e.g.

• https://svgwg.org/specs/paths/#DOMInterfaces if normalize is true then you have to turn arcs into beziers. Firefox does support that API but Chrome does not as yet: https://issues.chromium.org/issues/40441025

• I don't think we have anything that can work out the length of an arc without converting it to a bezier first: https://searchfox.org/firefox-main/source/dom/svg/SVGPathSegUtils.cpp#185 maybe there's something in Chrome we could adapt for that.

So if we went with drawing arcs as arcs we'd potentially have a disconnect when it comes to normalisation and/or length calculation.

Fixing the SVGArcConverter would be much preferable to skipping arc -> bezier conversion because of the above issues.

Comment 32

[Glenn Watson [:gw]]

Nical, Lee, could you through comments 30, 31 and let me know how you think we should progress with this? It's way outside my area of knowledge.

Comment 33

[Nicolas Silva [:nical]]

Attachment: Bug 1544413 - Work around a float precision issue in SVGArcConverter. r=#gfx-reviewers

Comment 34

[Robert Longson [:longsonr]]

Unfortunately most of the comments from comment 23 onwards are not the bug originally reported.

Comment 35

[Robert Longson [:longsonr]]

Attachment: WPT

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to Robert Longson [:longsonr] from comment #34)

Unfortunately most of the comments from comment 23 onwards are not the bug originally reported.

Darn. Should we move the patch here to a new bug even though that separates it from the debugging comments, or land the patch here and file a new bug for the original testcase?

Comment 37

[Robert Longson [:longsonr]]

I think we should probably leave this bug alone and move everything from comment 23 to a new bug as much as we can.

Comment 38

[Daniel Holbert [:dholbert]]

I can file a new bug for comment 23 onward when I'm at my desk in a couple hours.

Comment 39

[Daniel Holbert [:dholbert]]

I spun off bug 1988564 for comment 23 through this one. (which is about the issue in pacjent.gov.pl reported in https://github.com/webcompat/web-bugs/issues/101970 , and which is fixed by the attached phab patch).

nical, would you mind moving the patch to that bug, and we can leave this bug tracking the testcase in comment 0?

Comment 40

[Phabricator Automation]

Comment on attachment 9512513 [details]
Bug 1544413 - Work around a float precision issue in SVGArcConverter. r=#gfx-reviewers

Revision D264379 was moved to bug 1988564. Setting attachment 9512513 [details] to obsolete.

Comment 41

[Daniel Holbert [:dholbert]]

(I moved https://github.com/webcompat/web-bugs/issues/98608 to its own site-report bug, bug 1988904, BTW.)

Comment 42

[Glenn Watson [:gw]]

A summary of where we are at with this from a webcompat perspective:

Bug #1544413 (this bug) - Test case is not from a real web site, so I don't think it's relevant for webcompat itself. However several real world bugs were linked to this bug, assuming same behavior, described below:

Bug #1988564 - A real world bug on a Polish government website (https://github.com/webcompat/web-bugs/issues/101970). Fixed by https://phabricator.services.mozilla.com/D264379, handling a float accuracy issue in SVGArcConverter, similar to how Skia does it.

Bug #1836315 - A real world bug on GDocs. Diagnosed to a fundamental issue in how layout deals with the scale values in comments 13 & 14. Tim is going to land a small fix that will help in this specific case, but doesn't solve the general issue. Has been downgraded to an S4 by dholbert.

Bug #1988904 - This may have been fixed by #1988564. Is not a high volume site, so severity for webcompat is 3.

The other duplicated bugs appear to be 6-10 years old.

That is to say - I don't think there is any high priority work for webcompat that remains for this, and we should be able to leave this open for now and focus on some other high priority items. Anyone disagree or have specific webcompat pages linked from here that are a high priority?

Comment 43

[Daniel Holbert [:dholbert]]

(In reply to Glenn Watson [:gw] from comment #42)

Bug #1988904 - This may have been fixed by #1988564. Is not a high volume site, so severity for webcompat is 3.

(I don't think that one was fixed by bug 1988564, unfortunately. But indeed, not a high-volume site.)

I agree with the rest of your comment. Thanks!

Comment 44

[thomas.smailus]

(In reply to Glenn Watson [:gw] from comment #42)

A summary of where we are at with this from a webcompat perspective:

Bug #1544413 (this bug) - Test case is not from a real web site, so I don't think it's relevant for webcompat itself. However several real world bugs were linked to this bug, assuming same behavior, described below:

For what it’s worth , LOT of SVG graphic files and their applications sit inside corporate networks not accessible to the public internet. And the files cannot be shared publicly because they represent controlled technical information , such as in technical drawings. So creating a proxy graphic that illustrates the issue is the only option.

Comment 45

[Glenn Watson [:gw]]

(In reply to thomas.smailus from comment #44)

(In reply to Glenn Watson [:gw] from comment #42)

A summary of where we are at with this from a webcompat perspective:

Bug #1544413 (this bug) - Test case is not from a real web site, so I don't think it's relevant for webcompat itself. However several real world bugs were linked to this bug, assuming same behavior, described below:

For what it’s worth , LOT of SVG graphic files and their applications sit inside corporate networks not accessible to the public internet. And the files cannot be shared publicly because they represent controlled technical information , such as in technical drawings. So creating a proxy graphic that illustrates the issue is the only option.

That's a fair point! If that is the case here, or if there are some other linked cases where we have a test case that affects real world non-public content, please let us know and we can get the webcompat team to triage priority as appropriate.