Closed Bug 1599248 Opened 3 months ago Closed 3 months ago

Janky zoom on Daily Mail page

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox72 --- fixed
firefox73 --- verified

People

(Reporter: yoasif, Assigned: jnicol)

References

()

Details

(Keywords: nightly-community)

Attachments

(2 files)

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

Attached file about:support

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

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

Flags: needinfo?(yoasif)

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.

(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

Flags: needinfo?(yoasif)

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

(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).

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

Flags: needinfo?(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.

Flags: needinfo?(jnicol)
See Also: → 1598380

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.

Assignee: nobody → jnicol
Priority: -- → P2

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

Keywords: regression

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

Keywords: regression
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
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Does fenix need this on 72?

Flags: needinfo?(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
Flags: needinfo?(jnicol)
Attachment #9113266 - Flags: approval-mozilla-beta?

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 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

Attachment #9113266 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

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.

Laurentiu this is fixed now.

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.

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?

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!

Jamie, it works a lot better now. Thanks!

You need to log in before you can comment on or make changes to this bug.