1599248 - Janky zoom on Daily Mail page

Status: RESOLVED FIXED

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

Description

[Asif Youssuff]

Steps to reproduce:

1. Open Fenix (I tried GeckoView Example, but the page seems to go to a mobile view, while it opens as desktop view on Fenix)
2. Navigate to https://www.dailymail.co.uk/news/article-7723261/WeWork-founder-Adam-Nuemann-claimed-Mohammed-bin-Salman-Jared-Kushner-save-world.html
3. Double tap on main content area on left side

What happens:

Very slow, janky zoom to fit content.

Expected result:

Fast zoom to fit like on Chromium.

Profile: https://perfht.ml/2KTYNIM

Running on:

Nightly 191124 18:01 (Build #13281808)
📦: 23.0.0, 7f57dffad
🦎: 72.0a1-20191119043902

Comment 1

[Asif Youssuff]

Attachment: about:support

Comment 2

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

:yoasif, if you think that's a regression, then could you try to find a regression range in using for example mozregression?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Asif, can you RenderBackend,WrWorker in the Custom thread names in the profiler?

Comment 4

[Markus Stange [:mstange]]

We have a flag that avoids rerasterizing glyphs while the user is pinch-zooming. Presumably that flag isn't being set during the double-tap-to-zoom animation, which is driven by APZ.

Comment 5

[Asif Youssuff]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)

Asif, can you RenderBackend,WrWorker in the Custom thread names in the profiler?

Hope this helps: https://perfht.ml/37BDjtT

Comment 6

[Asif Youssuff]

I decreased the sampling interval and added screenshots -- maybe this will be more helpful: https://perfht.ml/2XJ8RK0

Comment 7

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #4)

We have a flag that avoids rerasterizing glyphs while the user is pinch-zooming. Presumably that flag isn't being set during the double-tap-to-zoom animation, which is driven by APZ.

Fixing that could just be a matter of changing this method to return mState == PINCHING || mState == ANIMATING_ZOOM (and perhaps renaming the method to IsZoomingInProgress() or something like that).

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Interestingly enough the profiles don't really show much glyph rasterization just a texture upload catastrophe.

Comment 9

[Jamie Nicol [:jnicol]]

Botond's suggested fix hugely improves things, but the first frame of the zoom is still a bit slow. That is due to bug 1598380. So let's make this bug about fixing double-tap-to-zoom to work like pinch-zooming, and deal with the remaining slowness in the other bug.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 1599248 - Use local raster space when animating a double tap zoom. r?botond

In bug 1531142 we made it so that when a spatial node is being pinch-zoomed we
use a local raster-space to avoid rerasterizing glyphs for every slight change
in zoom level. This makes it so that we also apply the same trick when
being asynchronously zoomed by a double-tap gesture.

Comment 11

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 12

[Jamie Nicol [:jnicol]]

Not a regression - only affects webrender on android, and always has done so

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37582d3167c5
Use local raster space when animating a double tap zoom. r=botond

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/37582d3167c5

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

Does fenix need this on 72?

Comment 16

[Jamie Nicol [:jnicol]]

Comment on attachment 9113266 [details]
Bug 1599248 - Use local raster space when animating a double tap zoom. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: Terrible performance when double-tap zooming a page on Fenix. Only applies to when webrender is enabled (which will be the case for Pixel 2, Pixel 2 XL, Pixel 3, and Pixel 3 XL devices on geckoview 72).
• Is this code covered by automated tests?: No
• 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): Uses existing code path for pinch zooming, simply enables it for double tap zooming too. Only affects webrender so a limited population on android.
• String changes made/needed: N/A

Comment 17

[Jamie Nicol [:jnicol]]

Yes, this would be great to have for 72. I answered no for "verified in nightly", because it may be a few days before a Fenix nightly includes this fix. But I have verified it in local builds of geckoview_example.

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9113266 [details]
Bug 1599248 - Use local raster space when animating a double tap zoom. r?botond

fix for webrender android, approved for 72.0b4

Comment 19

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

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

Comment 20

[Laurentiu Apahidean]

I tested the issue on the latest Firefox Preview Nighlty build 191206 6:01 (Build #13430608) 72.0a1-20191202091209 using a Pixel 3 XL (Android 9) and the issue still occurs if performing the following steps.

1. Go to about:config and set gfx.webrender.all to ture;
2. Navigate to https://www.dailymail.co.uk/news/article-7723261/WeWork-founder-Adam-Nuemann-claimed-Mohammed-bin-Salman-Jared-Kushner-save-world.html;
3. Request desktop site;
4. Double tap anywhere on the page in order to zoom in.

Comment 21

[Jamie Nicol [:jnicol]]

Firefox Preview Nightly hasn't yet been updated to include a version of gecko with the fix. You can see the build ID is 2019-12-02, Dec 2nd. It'll need to be Dec 5th or later before this fix is included.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Laurentiu this is fixed now.

Comment 23

[Laurentiu Apahidean]

I have retested the issue using a Pixel 3 XL (Android 9) on Firefox Preview Nightly build 191212 (Build #13460608) 73.0a1-20191211094640 and the issue no longer occurs. When zooming in by performing a double tap the action is smooth without any jerkiness.

Comment 24

[Asif Youssuff]

It is faster on my device (Pixel 2), but still janks the first time I do it after initial page load. Zooming out and double tapping again is much smoother (jank free). Should I open a new bug and take a new profile?

Comment 25

[Jamie Nicol [:jnicol]]

That was the expected behaviour when the patch for this bug landed, but before bug 1598380 landed. It briefly landed, then was backed out, then landed again, so I'm not sure if it was in place when you last tested. Asif, could you re-test with Fenix nightly and see if it's better now? It certainly is for me. A profile would be great if it's still janky!

Comment 26

[Asif Youssuff]

Jamie, it works a lot better now. Thanks!