1756750 - OTF bitmap font in SVG does not always render properly on first paint

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Frank]

Attachment: bitmap_otf_issue.html

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

Steps to reproduce:

1. Clear browser cache
2. Load an OTF bitmap font on a webpage using the @font-face notation
3. Use the OTF bitmap font in a tspan node within SVG by specifying the font-family
4. Open the webpage

Actual results:

The rendered text is either not shown at all, is partially shown or completely shown. This changes per refresh. Selecting the text seems to cause a repaint, sometimes fixing the issue until the page is refreshed.

Expected results:

The text should always be fully rendered

Comment 1

[Frank]

This issue was not present in the 94.0.x versions, but is present in 95.0.x

Comment 2

[Robert Longson [:longsonr]]

Would you mind using https://mozilla.github.io/mozregression/ to tell us exactly when it changed?

Comment 3

[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 revert this change in case you think the bot is wrong.

Comment 4

[Frank]

2022-02-23T10:42:57.470000: DEBUG : Found commit message: Bug 1712780 - Skip running the line-breaker when scanning SVG text frames. r=emilio

As we don't support multi-line text in SVG, there's no point running the line-breaker and
collecting potential break positions for textframes that are part of an SVG text subtree.
Telling BuildTextRunsScanner to skip this makes it somewhat less expensive.

In my local build, this reduces the reflow time of the testcase from nearly 20s to about 4.5s.
Still much too long, but at least it's a step in the right direction.

Differential Revision: https://phabricator.services.mozilla.com/D129404

2022-02-23T10:42:57.470000: DEBUG : Did not find a branch, checking all integration branches
2022-02-23T10:42:57.474000: INFO : The bisection is done.
2022-02-23T10:42:57.475000: INFO : Stopped

Comment 5

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

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

Comment 6

[Pascal Chevrel:pascalc]

Jonathan, fyi, this is a regression from bug 1712780.

Comment 7

[Jonathan Kew [:jfkthame]]

That's quite puzzling, but I can definitely reproduce the problem. I guess it may have something to do with failing to record that the SVG text needs to be refreshed once the @font-face load completes, because it is initially invisibly due to the pending font download.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Do you have time to look into this Jonathan? Otherwise feel free to ni? me and I can take it.

Comment 9

[Jonathan Kew [:jfkthame]]

I probably won't get to this for a while, at least, so if you can find the cycles to take a look that'd be great - thanks!

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(That took a fair bit of digging)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1756750 - Don't reframe <svg:image> on loading / broken changes. r=jfkthame

Doesn't change behavior but makes debugging easier and should be a perf
improvement anyways.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1756750 - Ensure we observe SVG glyph changes inside <svg:text>. r=jfkthame

This gets done usually in BreakSink::Finish, but we don't do
line-breaking in SVG Text so we need to do this here instead.

Do you know where I could crib a test for this?

Depends on D139964

Comment 13

[Jonathan Kew [:jfkthame]]

Nice bit of digging! So this happened because the font was not just a bitmap font, but an SVG-in-OT font with PNG bitmaps embedded in the SVG glyph documents. Those PNGs get decoded asynchronously when we first ask to render the SVGs, and so although the font isn't "animated" as such, we depend on updating the rendering when we get notified that decoding is finished.

(One confirmation of this is that setting image.decode-immediately.enabled to true avoids the issue.)

I'll see if I can hunt down (or make) a suitable font for a testcase; don't think we have anything like this currently in-tree or in WPT.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1756750 - Reftest for SVG-in-OT font with embedded bitmaps. r=emilio

The testcase includes a 1-second pause after the onload event fires, because the test font
includes PNG bitmaps which are asynchronously decoded. This is in theory unreliable,
but for these small images it should be plenty of time (even without any delay, the test
fails only very intermittently) -- if we're failing to decode the images even with the
1-second delay, something is probably truly broken.

(The file svg-bitmap.ttx file is not actually required to run the test, but is the source
for the svg-bitmap.ttf font file, constructed by inserting base64-encoded PNG images
into a minimal font framework.)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a704188b5125
Don't reframe <svg:image> on loading / broken changes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/e48e94cfea71
Ensure we observe SVG glyph changes inside <svg:text>. r=jfkthame

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63d6f4fa78da
Add a missing null-check.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa7a48cff13c
Reftest for SVG-in-OT font with embedded bitmaps. r=emilio

Comment 18

[Sandor Molnar[:smolnar]]

Backed out changeset aa7a48cff13c (bug 1756750) for causing reftest failures layout/reftests/text-svgglyphs/svg-in-ot-bitmap-1.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/e0bde4732992e3a0741eaa0fd7f45158d1e84456

Push with failures

Failure log

INFO - #72: ??? (???:???)
[task 2022-03-02T19:03:28.915Z] 19:03:28     INFO - [Child 6288, Main Thread] ###!!! ASSERTION: The context of the current AzureState is not altered after Save() been called. you may consider to remove this pair of gfxContext::Save/Restore.: 'CurrentState().mContentChanged || CurrentState().pushedClips.Length() > 0', file /builds/worker/checkouts/gecko/gfx/thebes/gfxContext.cpp:152
[task 2022-03-02T19:03:28.915Z] 19:03:28     INFO - #01: NS_DebugBreak [xpcom/base/nsDebugImpl.cpp:429]

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Yeah I saw this assert fire locally when debugging the issue... I think it's harmless tho, nad it's expected if the image hasn't loaded yet. Probably should be turned into a warning, WDYT Jonathan?

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a704188b5125
https://hg.mozilla.org/mozilla-central/rev/e48e94cfea71
https://hg.mozilla.org/mozilla-central/rev/63d6f4fa78da

Comment 21

[Jonathan Kew [:jfkthame]]

It seems basically harmless, but it was probably added in order to flag optimization opportunities, and I suspect weakening it to a warning will in effect mean it never gets noticed.

Is the assertion firing here because we're using requestAnimationFrame() but not actually modifying anything in the callback, so it's warning us that we're doing pointless work? I wonder if we can avoid it by actually making rAF() do something meaningful (and restoring the original state for the eventual reftest snapshot). Pushed https://treeherder.mozilla.org/jobs?repo=try&revision=29c65437f22a708900708a18fc082f4ef7d8e578 to see what that shows.

Comment 22

[Jonathan Kew [:jfkthame]]

Ah, I see.... the assertion comes from within the individual gfxSVGGlyphs::RenderGlyph calls. So comment 21 doesn't help, as within the individual glyph the save/restore is still redundant as long as the PNG isn't decoded yet.

Given that gfxContextAutoSaveRestore is used in a number of other places as well, I'm hesitant to remove the assertion from it; instead, I suggest we simply don't use the RAII helper here in RenderGlyph, but do a manual Save/Restore pair (with a comment about the reason). There are no early-returns or other tricky code paths to worry about, so while the RAII approach may be more elegant, it's not really giving us any significant value here.

[edit] Oops - misread the stack - the assertion doesn't come from gfxContextAutoSaveRestore but from Restore itself. So that won't help.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/899a4ee58f6d
Reftest for SVG-in-OT font with embedded bitmaps. r=emilio

Comment 24

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/899a4ee58f6d

Comment 25

[Monica Chiorean]

I was able to reproduce the issue on Win10 using build 99.0a1 (20220225104705), the issue HTML page did not load at all.
Verified as fixed on Win10/Ubuntu 20.4/Mac 10.13 using builds 99.0b4(20220315185755) and 100.0a1(20220316214937).