Janky zoom on Daily Mail page
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
People
(Reporter: yoasif, Assigned: jnicol)
References
()
Details
(Keywords: nightly-community)
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details |
Steps to reproduce:
- Open Fenix (I tried GeckoView Example, but the page seems to go to a mobile view, while it opens as desktop view on Fenix)
- Navigate to https://www.dailymail.co.uk/news/article-7723261/WeWork-founder-Adam-Nuemann-claimed-Mohammed-bin-Salman-Jared-Kushner-save-world.html
- 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
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Comment 2•5 years ago
|
||
:yoasif, if you think that's a regression, then could you try to find a regression range in using for example mozregression?
Comment 3•5 years ago
|
||
Asif, can you RenderBackend,WrWorker in the Custom thread names in the profiler?
Comment 4•5 years ago
|
||
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.
Updated•5 years ago
|
Reporter | ||
Comment 5•5 years ago
|
||
(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
Reporter | ||
Comment 6•5 years ago
|
||
I decreased the sampling interval and added screenshots -- maybe this will be more helpful: https://perfht.ml/2XJ8RK0
Comment 7•5 years ago
|
||
(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•5 years ago
|
||
Interestingly enough the profiles don't really show much glyph rasterization just a texture upload catastrophe.
Assignee | ||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 11•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Assignee | ||
Comment 12•5 years ago
|
||
Not a regression - only affects webrender on android, and always has done so
Comment 13•5 years ago
|
||
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/37582d3167c5 Use local raster space when animating a double tap zoom. r=botond
Comment 14•5 years ago
|
||
bugherder |
Assignee | ||
Comment 16•5 years ago
|
||
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
Assignee | ||
Comment 17•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
bugherder uplift |
Comment 20•5 years ago
|
||
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.
- Go to about:config and set gfx.webrender.all to ture;
- Navigate to https://www.dailymail.co.uk/news/article-7723261/WeWork-founder-Adam-Nuemann-claimed-Mohammed-bin-Salman-Jared-Kushner-save-world.html;
- Request desktop site;
- Double tap anywhere on the page in order to zoom in.
Assignee | ||
Comment 21•5 years ago
|
||
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 23•5 years ago
|
||
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.
Reporter | ||
Comment 24•5 years ago
|
||
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?
Assignee | ||
Comment 25•5 years ago
|
||
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!
Description
•