1780191 - thin SVG rect disappearing

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect)

Description

[lxpugin]

Attachment: Example SVG file and output with FF 102, 101 and 100

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

Steps to reproduce:

SVG files with <rect> within a viewBox do not show anymore; scaling is different than with previous versions of Firefox
UA: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Firefox/102.0
To reproduce, display the SVG file example.svg in the archive

Actual results:

<rect> can disappear, although not all of them (see attached file 102.0.1.png)

Expected results:

Was working in previous version of Firefox (see attached files with 101.0.2.png and 100.0.1.png)

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::SVG' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[lxpugin]

The example file available here

Comment 3

[Alice0775 White]

I can reproduce the issue in Nightly104.0a1 Windows10.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cddcdc46d3484be662cc56ceefe6b282481ca115&tochange=950227a483519fe71798772a0996c02dec2125f1

Comment 4

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

:nical, since you are the author of the regressor, bug 1686654, could you take a look?
For more information, please visit auto_nag documentation.

Comment 5

[Jim Mathies [:jimm]]

Maybe S2 depending on how widespread it is. Waiting on Nical for feedback / severity update.

Comment 6

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

Set release status flags based on info from the regressing bug 1686654

Comment 7

[Jeff Muizelaar [:jrmuizel]]

This doesn't seem to be affecting real content. Lowering the severity.

Comment 8

[lxpugin]

This doesn't seem to be affecting real content. Lowering the severity.

What do you mean "not affecting real content"?? We have been using SVG in real world applications for many years and it has been working without problem in all browsers. The SVG is used to display music scores. Some of the content disappear when rendered with Firefox, which makes it unusable. This is a new issue in Firefox. Do you need more information?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

lxpugin do you have an example URL that runs into this bug?

Comment 10

[lxpugin]

Thank you for reconsidering the severity change. This is one example https://www.verovio.org/mei-viewer.xhtml

If you open the panel on the right and zoom out you will see the some of the vertical lines attached to the music notes disappear.

Comment 11

[andrew.hankinson]

I can confirm that this is also a problem on Firefox 104 (macOS 12.5).

It happens only at certain sizes and offsets. To reproduce, load the example in comment #2 above, and "pinch zoom" in-and-out. You will see the vertical lines on the notes (the "stems") blinking on and off as you zoom.

I've made an image comparing the different browsers at roughly the same zoom levels, and then have highlighted some areas in the image where the stems have disappeared in Firefox, but display correctly in Safari (Both Safari and Chrome do not exhibit this same "blinking" behaviour when pinch-zooming.)

Comment 12

[andrew.hankinson]

The image did not seem to embed correctly, so here it is in a gist: https://gist.github.com/ahankinson/581ca11406dc83acc79b2fc55d3a6566

Comment 13

[Nicolas Silva [:nical]]

I think that this was caused by the same issue as bug 1792313. I can still reproduce the problem in release (which does not have the fix)but the test case displays properly on nightly.

Comment 15

[lxpugin]

I just checked with 107.0b9 and nightly 108.0a1 (2022-11-12) on Mac it is not fixed.

Comment 16

[lxpugin]

Here is a GIF (rendering with the nightly mentioned above) illustrating that the problem is still there
https://gist.githubusercontent.com/ahankinson/581ca11406dc83acc79b2fc55d3a6566/raw/06ae7af29ea28b192948f5b06bcc40e2a3a4c64e/disappear.gif

Comment 17

[Alice0775 White]

Yes, I can also reproduce this issue on Firefox107.0RC(build 20221110173214) Nightry108.0a1(build 20221114085151) Windows10.

STR:

1. open https://gist.githubusercontent.com/lpugin/61bcde6a97bfca6a14fdd2a5f66e5ded/raw/4ec44185288d94c69ecd777145c1f24595578a85/example.svg
2. Zoom in several times if needed

Comment 18

[Jim Mathies [:jimm]]

Requesting re-triage. Not sure if this is something we should be tracking.

Comment 19

[Ashley Hale [:ahale]]

Just taking a look at this as part of bug triage discussion, I presume that the issue is that SVG rendering is using a simple pixel overlap criteria and at some zoom levels the pixel centers happen to be missing the vertical bars (neighboring pixels being left of, and right of, but never exactly on the bars) entirely at some zoom levels, where it would be more ideal if they were antialiased so they would fade to a shade of gray at these zoom levels.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1780191. Avoid making primitives smaller than one pixel active. r?nical

Webrender doesn't seem to handle them well. This of course will affect the balance of making things active, but hopefully this is a good trade off.

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Ashley Hale [:ahale] from comment #19)

Just taking a look at this as part of bug triage discussion, I presume that the issue is that SVG rendering is using a simple pixel overlap criteria and at some zoom levels the pixel centers happen to be missing the vertical bars (neighboring pixels being left of, and right of, but never exactly on the bars) entirely at some zoom levels, where it would be more ideal if they were antialiased so they would fade to a shade of gray at these zoom levels.

They should be antialiased already, we push a rect here with the aForceAntiAliasing parameter true

https://searchfox.org/mozilla-central/rev/20fc3d71e6a295984a645403249610ecc7b90c0c/layout/svg/SVGGeometryFrame.cpp#838

So it's some bug with snapping inside webrender maybe?

Comment 22

[Ashley Hale [:ahale]]

Thinking more broadly about this, maybe rect and most other element types in SVG should simply not be snapped at all?

Comment 23

[Timothy Nikkel (:tnikkel)]

Maybe! I'm not sure. I'm not very familiar with webrender snapping, but from vaguely following the webrender snapping work over the years it seems like a complicated topic with solutions that are either not simple or would upset a balance, so I chose not to jump into that sea.

Comment 24

[Nicolas Silva [:nical]]

If SVG rects are snapped in WR then that's a bug. I think that if it's not already the case, the logic should be to potentially snap if aForceAntialiasing is false (ther might be other things that cause us to not snap, like some types of transforms, I don't know) and never snap if aForceAntialiasing is true.

Comment 25

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tnikkel, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 26

[Timothy Nikkel (:tnikkel)]

I need to write a test.

Comment 27

[Timothy Nikkel (:tnikkel)]

Wrote a test for this last week and updated the revision. I'll push this after the soft freeze.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d75b2841884f
Avoid making primitives smaller than one pixel active. r=nical

Comment 30

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9310214 [details]
Bug 1780191. Avoid making primitives smaller than one pixel active. r?nical

Beta/Release Uplift Approval Request

• User impact if declined: one pixel wide svg rects don't draw (common in music notation on web)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): small change to heuristics
• String changes made/needed:
• Is Android affected?: Yes

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 32

[Donal Meehan [:dmeehan]]

Comment on attachment 9310214 [details]
Bug 1780191. Avoid making primitives smaller than one pixel active. r?nical

Approved for 113.0b4.

Comment 33

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d2def7511e8a

Comment 34

[Dianna Smith [:diannaS]]

:tnikkel is this safe enough to uplift to release for 112 dot release? given the regression introduced by bug 1811978 in 112?

Comment 35

[Timothy Nikkel (:tnikkel)]

Yes, I'd be okay with that.

Comment 36

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9310214 [details]
Bug 1780191. Avoid making primitives smaller than one pixel active. r?nical

Beta/Release Uplift Approval Request

• User impact if declined: one pixel wide svg rects don't draw (common in music notation on web)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): small change to heuristics
• String changes made/needed:
• Is Android affected?: Yes

Comment 37

[Anca Soncutean, Desktop QA]

Reproduced with Fx 112.0a1 (2023-03-02) on Windows 10.
Verified fixed with Fx 114.0a1 (2023-04-18) and Fx 113.0b5 Windows 10, macOS 13 and Ubuntu 22.

Comment 38

[Dianna Smith [:diannaS]]

turns out this actually doesnt graft cleanly to release due to the dependency/conflicts of bug 1820752 (which is in 113). let me know if there is a simple conflict resolution, if not it can just ride the 113 trains.

Comment 39

[Timothy Nikkel (:tnikkel)]

I will make a patch for release, it is simple to do so.

Comment 40

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1780191. Avoid making primitives smaller than one pixel active. r=nical

Webrender doesn't seem to handle them well. This of course will affect the balance of making things active, but hopefully this is a good trade off.

Comment 41

[Timothy Nikkel (:tnikkel)]

Okay, patch that applies cleanly to release is up. The fuzz value on the test might need to be adjusted a bit, just waiting on try results for that.

Comment 42

[Timothy Nikkel (:tnikkel)]

The fuzz seems fine based on my try run (pushing based on the revision when 112 left mozilla-central), but landing it directly on release might still be different, so a small fuzz update might still be needed.

Comment 43

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9329499 [details]
Bug 1780191. Avoid making primitives smaller than one pixel active. r=nical

Beta/Release Uplift Approval Request

• User impact if declined: one pixel wide svg rects don't draw (common in music notation on web)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): small change to heuristics
• String changes made/needed:
• Is Android affected?: Yes

Comment 44

[Dianna Smith [:diannaS]]

Comment on attachment 9329499 [details]
Bug 1780191. Avoid making primitives smaller than one pixel active. r=nical

Approved for 112.0.2 dot release

Comment 45

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/542e7cca389c

Comment 46

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.