Closed Bug 1684520 Opened 4 years ago Closed 3 years ago

Scrolling via keyboard visibly stutters more than before

Categories

(Core :: Panning and Zooming, defect, P2)

defect

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox84 --- wontfix
firefox85 --- wontfix
firefox86 --- wontfix
firefox87 --- wontfix
firefox88 --- fixed

People

(Reporter: rmader, Assigned: tnikkel, NeedInfo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

This was bisected in a discussion in bug 1601962 comment 7 to bug 1601962 comment 10:

Scrolling via keyboard appears to be visibly slower than before bug 1657822 - most importantly < 60fps.

Bisected on Linux, but apparently also applies to Windows.

Blocks: 1601962

P.S.: setting apz.force_disable_desktop_zooming_scrollbars:true makes scrolling smooth again, even though with elements getting rendered slowly.

That change made keyboard scrolling be performed by apz instead of the main thread. If you did apz based scrolling (say from a mouse wheel or touchpad) I would expect it to look similar. Is that the case?

Flags: needinfo?(robert.mader)

Personally I'd say keyboard scrolling is more choppy (also see bug 1601962 comment 13). I'll attach some profiles, scrolling on bug 1601962, maybe that helps?

  1. touchpad: https://share.firefox.dev/2WYVhm0
  2. keyboard: https://share.firefox.dev/3pB6khz
  3. keyboard with apz.force_disable_desktop_zooming_scrollbars:true: https://share.firefox.dev/2LbzsNp

Given that in the 3. case rendering doesn't keep up with scrolling, there's of course the possibility that WR is just too slow here? Would surprise me though.

Flags: needinfo?(robert.mader)

(In reply to Robert Mader [:rmader] from comment #3)

Personally I'd say keyboard scrolling is more choppy (also see bug 1601962 comment 13). I'll attach some profiles, scrolling on bug 1601962, maybe that helps?

  1. touchpad: https://share.firefox.dev/2WYVhm0
  2. keyboard: https://share.firefox.dev/3pB6khz
  3. keyboard with apz.force_disable_desktop_zooming_scrollbars:true: https://share.firefox.dev/2LbzsNp

Given that in the 3. case rendering doesn't keep up with scrolling, there's of course the possibility that WR is just too slow here? Would surprise me though.

Thanks for that.

Interesting. Comparing the last two there is a lot more displaylist building happening in the content process in 2 then in 3. So maybe apz controlling and updating the main thread scroll position means we can't avoid a display list build whereas if the main thread controls the scroll position it can avoid the display list build?

What do profiles 2 and 3 looks like if you use page up/down or space bar to scroll instead of many arrow key presses?

It appears to me smooth then, with apz giving the better results as there are no rendering artefacts:

  1. default: https://share.firefox.dev/3aRAUzv
  2. disabled apz: https://share.firefox.dev/34WntdS

From https://www.reddit.com/r/firefox/comments/kojli8/

Plus, verified that layers.async-pan-zoom.enabled (defaults to enabled) hinders scrolling via keyboard for specific websites. It can result in as much as -50% FPS under Webrender and around -25% FPS under Direct3D 11 Advanced Layers, Direct3D 11, and Basic. Less FPS, choppier scrolling compared to the mousewheel.
Can someone explain to me how do you code something like this? (bug in action)
Slide view horizontally one notch to the right - no more performance loss when scrolling via keyboard. Slide view back (or higher res without horizontal scrollbar) - minus 50%FPS!
It's https://wiki.mozilla.org/Platform/GFX/perf_triage, not a random website..

Even here (bugzilla), reddit, github, microsoft, stackoverflow, twitter, imgur, instagram - scrolling via keyboard halves the fps.
about:support, raw view, forums.mydigitallife.net, guru3d.com, wccftech.com - does not.

Turning apz off solves it in release.
On nightly, it solves it, but sometimes causes a black rectangle the size of the viewport sliding into view (goes away just by opening devtools via f12 and closing them). Hopefully it wont get into next stable, because the only other option is to use apz.force_disable_desktop_zooming_scrollbars true and that one breaks rendering.

Frankly, not worth the trouble on a desktop with a mouse and keyboard setup.
Not even worth it for touchpad users, where driver and os enhancements already covered it.

(In reply to Mos At from comment #7)

because the only other option is to use apz.force_disable_desktop_zooming_scrollbars true and that one breaks rendering.

Can you expand on this? How does that pref break rendering?

Flags: needinfo?(atmos2020)

(In reply to Timothy Nikkel (:tnikkel) from comment #8)

(In reply to Mos At from comment #7)

because the only other option is to use apz.force_disable_desktop_zooming_scrollbars true and that one breaks rendering.

Can you expand on this? How does that pref break rendering?

you will see a blank space on top or bottom depending on direction, keep trying to catch up loading content.

its extremely painful to look at, more so than trash fps

(In reply to Timothy Nikkel (:tnikkel) from comment #8)

Can you expand on this? How does that pref break rendering?

What howong said - preview

Flags: needinfo?(atmos2020)

Thanks for the answers. And how did keyboard scrolling work for you prior to the pref apz.force_disable_desktop_zooming_scrollbars existing, say in June or earlier of this year? Was it the same, better, worse, different?

Flags: needinfo?(howong)
Flags: needinfo?(atmos2020)

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

Thanks for the answers. And how did keyboard scrolling work for you prior to the pref apz.force_disable_desktop_zooming_scrollbars existing, say in June or earlier of this year? Was it the same, better, worse, different?

I don't recall ever being bothered by it before v80.
Then I've started using Webrender and figured it's one of it's quirks. Turns out it affects all other renders - only half as bad, though.
Some people noticed it after v82 and seems confirmed by the bisection mentioned in this thread, so "katz be dammned".

Now that I need to work more with the keyboard editing stuff and not just lazily scroll past pages with the mousewheel, I can't stand it anymore.
And the baffling randomness of it per website / viewport as showcased above is something I do not need to worry about, so I'm inclined to dump APZ - the only workaround without glitches. On desktop with a mouse and keyboard anyway, and these prefs provide amazing smooth scrolling for both reading and skipping.

I have low expectations of an actual fix for keyboard scrolling with APZ on, and nightly worries me that turning off APZ might glitch in the next stable releases. A man can hope.

Flags: needinfo?(atmos2020)

Hi just wanted to confirm that this choppy keyboard scrolling affects me too on different Linux machines.
As i couldn't find any info i've bisected it via mozregression-gui to this Bug 1657822. Before the change to (Enable new desktop zooming scrollbar code) - keyboard scrolling was smooth. Mouse smooth scrolling is fine!
Regards

Depends on: 1684776
Severity: -- → S2
Priority: -- → P2
Flags: needinfo?(tnikkel)

I can't really see a difference of smoothness in scrolling between using the scroll wheel or the keyboard, but I'd like to verify this issue when it gets fixed. If it needs verification, please NI me and provide some reproduction steps. I assume I'd need a high-resolution monitor and graphics card to properly recognize the stuttering.

(In reply to Robert Mader [:rmader] from comment #3)

Personally I'd say keyboard scrolling is more choppy (also see bug 1601962 comment 13). I'll attach some profiles, scrolling on bug 1601962, maybe that helps?

  1. touchpad: https://share.firefox.dev/2WYVhm0
  2. keyboard: https://share.firefox.dev/3pB6khz
  3. keyboard with apz.force_disable_desktop_zooming_scrollbars:true: https://share.firefox.dev/2LbzsNp

Given that in the 3. case rendering doesn't keep up with scrolling, there's of course the possibility that WR is just too slow here? Would surprise me though.

By keyboard here you mean pressing the arrow key many times? Or holding it down?

I've tried to reproduce a profile like this one (where there is a stack captured asking for display list building but no display list building marker in the common case) and I cannot do it. Inspecting the code I also cannot figure out how we don't hit the display list building profile marker.

Flags: needinfo?(robert.mader)

Also, what page are you scrolling?

It's about keeping the keyboard arrow buttons pressed (continuous input). It's not about heavy site content, it already scrolls choppily on the most lightweight sites.

As walmartguy pointed out it appears while holding up/down. For bisecting the issue I usd some bugzilla bug - on FF stable I can reproduce it on this site here.

If it helps I can provide some fresh profiles from nightly, maybe things have improved there.

Flags: needinfo?(robert.mader)

Could you upload a new profile of the keyboard scrolling with apz.force_disable_desktop_zooming_scrollbars:true? I've tried to reproduce what I see in that profile on a couple of Windows machines, a linux machine, and a few macs and I've been unable to.

Flags: needinfo?(robert.mader)

From todays nightly:

Flags: needinfo?(robert.mader)

Thanks. Those profiles are different the profiles in comment 3 in a key aspect: the profiles in comment 3 have much less (less than 50%) DisplayList markers with apz.force_disable_desktop_zooming_scrollbars:true (even when things in the profile show that they must exist for that refresh driver tick). The new profiles it's reversed, the apz.force_disable_desktop_zooming_scrollbars:true profiles have about 2x as many DisplayList markers.

When you observe on current nightly do you still observe the same difference in stuttering?

(In reply to Timothy Nikkel (:tnikkel) from comment #22)

When you observe on current nightly do you still observe the same difference in stuttering?

Yes. To put numbers on it using GALLIUM_HUD=fps firefox I get smooth 60fps with apz.force_disable_desktop_zooming_scrollbars:true while with default settings it's between 40-55 and never 60. I.e. lots of frame skipping.

Okay thanks for those numbers, I think I've found something in the profile that correspond to that. I'll see where it leads.

(edited later: this comment seems to be incorrect)

We have a running scroll animation on the compositor, we sample it, it produces reasonable (and non-zero) changes every time we sample it but the compositor still determines nothing has changed and the invalid region for the composite is empty. (The same problem happens with and without wr, so I'm debugging the non-wr version because it is slightly easier.)

It looks like it supposed to work by AsyncCompositionManager::TransformShadowTree first calling Sample on all running animations, and then AsyncCompositionManager::ApplyAsyncContentTransformToTree sets a new base shadow transform using the update values, and then ComputeDifferences with the cloned layer properties is supposed to generate an invalid region to composite, except that region is empty sometimes when the animation, and hence the new transform, changed by reasonable numbers (not possible they rounded/snapped down to zero). Not sure why yet, or if it's some other problem causing that.

I think this is because our sample state get confused. I'll try to explain by example. Our sample state always has two entries. Here is how a sequence of compositor transactions goes in the middle of doing the keyboard scrolling via pure relative smooth scroll updates sent to apz. The numbers given are the vertical scroll offset.

transaction 1
sampled state: 814, 814. metrics: 837.
sampled states have scroll offset 814 and 814 (we'll see why the sampled states have equal scroll offset later)
we pop one 814
we push a new state with scroll offset 837 that comes from our metrics, it was updated from 814 to 837 by the active scroll animation on a previous transaction.
we advance our scroll animation and update our metrics to 862.
sampled state: 814, 837. metrics: 862.

we get a notify layers updated call, it has a scrollOffsetUpdate (the purerelative smooth scroll request) so we update both sampled states to our metrics value (862)

transaction 2
sampled state: 862, 862. metrics: 862.
sampled states have scroll offset 862 and 862
we pop one 862
we push a new state with scroll offset 862 that comes from our metrics, it was updated from 837 to 862 by the active scroll animation on a previous transaction.
we advance our scroll animation and update our metrics to 885.
sampled state: 862, 862. metrics: 885.

transaction 3.
sampled state: 862, 862. metrics: 885.
sampled states have scroll offset 862 and 862
we pop one 862
we push a new state with scroll offset 885 that comes from our metrics, it was updated from 862 to 885 by the active scroll animation on a previous transaction.
we advance our scroll animation and update our metrics to 909.
We use the same scroll offset in transaction 3 as in transaction 2 (862) so the transaction is empty and we don't paint anything.

I think the problem here is that we treat a smooth scroll request as a scrollOffsetUpdate, but it doesn't update the scroll offset, it leaves it alone, it just starts a smooth scroll animation which will update the scroll offset in the future.

A smooth scroll request is a request to perform scrolling in the future, it does not indicate that the scroll offset has been updated. However we still need to run a few of the things from a scroll offset update.

Currently missing a test. I'm not sure how to test this, any ideas?

This seems kind of fragile. We could have more bugs of this type, I didn't look for them or audit the code, but we should.

Assignee: nobody → tnikkel
Status: NEW → ASSIGNED

Pasting this here so I can find it easily. Botond suggested a gtest to test this, this test might be a good starting off point https://searchfox.org/mozilla-central/rev/7bb1cc6abf6634b2a20f71935e1e519e73402b63/gfx/layers/apz/test/gtest/TestBasic.cpp#500

Flags: needinfo?(tnikkel)
Blocks: 1695418
Attached file Bug 1684520. Add test. r?botond (deleted) —
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dda5061e661e
Do not consider the scroll offset to be updated when we recieve a smooth scroll request. r=botond
https://hg.mozilla.org/integration/autoland/rev/4c266c259457
Add test. r=botond
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Flags: in-testsuite+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: