Closed Bug 1062331 Opened 10 years ago Closed 9 years ago

Turn on by default touch event smoothing with gfx.frameuniformity.hw-vsync and gfx.touch.resample

Categories

(Core Graveyard :: Widget: Gonk, defect)

26 Branch
x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
mozilla38
feature-b2g 2.2+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: milan, Assigned: mchang)

References

Details

Attachments

(8 files)

We need to do UX investigation then decide if we like the touch event smoothing and turn it on by default.
Start having it assigned to Gordon to get the decision.
Assignee: nobody → gbrander
feature-b2g: --- → 2.1
Toggled the following on in aurora:

user_pref("gfx.frameuniformity.hw-vsync", true);
user_pref("gfx.touch.resample", true);

I see no difference in hitching at low or high scrolling speeds. Am I doing something wrong?
Flags: needinfo?(mchang)
With a restart, and on Flame, I assume.
Gordon and I chatted about this and Gordon wanted to test after bug 1063158 landed. I did some user testing. While scrolling with one hand and naturally, it was difficult to tell if any difference at all. Slow scroll side by side, it was easy to tell the difference, but they remarked that they "never scrolled like that". Another person is user testing a taco, which is a higher end device. I noticed that it was very janky on a taco. I enabled the pref on the taco and scrolling got MUCH better. I wonder if it will only be very user noticeable on higher end devices. But since bug 1063158 just landed, Gordon, can you please try again on the contacts or settings app? Thanks!
Flags: needinfo?(mchang)
(In reply to Mason Chang [:mchang] from comment #4)
> Gordon and I chatted about this and Gordon wanted to test after bug 1063158
> landed. I did some user testing. While scrolling with one hand and
> naturally, it was difficult to tell if any difference at all. Slow scroll
> side by side, it was easy to tell the difference, but they remarked that
> they "never scrolled like that". Another person is user testing a taco,
> which is a higher end device. I noticed that it was very janky on a taco. I
> enabled the pref on the taco and scrolling got MUCH better. I wonder if it
> will only be very user noticeable on higher end devices. But since bug
> 1063158 just landed, Gordon, can you please try again on the contacts or
> settings app? Thanks!

Bug 1063158 did not land - it was closed as WONTFIX.

If the reason for waiting for bug 1063158 to be fixed was to fix the jankiness of the scrollbars, then I expect that to be fixed in bug 1061327 instead.
Just for informational purposes, I have started dogfooding this on my tako device. Scrolling is indeed buttery (silky) smooth. However, there are occasional periods where the jank level goes way up for a few seconds before switching back to smooth. I haven't been able to reproduce this consistently, but I will post here if I figure out how.
What you're describing sounds like what I would expect to happen when the main thread is busy, because the events will get blocked. Bug 930939 is the thing that should address that. However, what you're seeing in that scenario should be no worse than it is with the default behaviour today.
HD video with scrolling on two flames side by side. Left one has the patch.
HD video of scrolling settings
Due to the 10mb limit, had to upload this to dropbox.
Looks good per video. On device I could tell no difference, but that's not the same thing as a regression.

Buttery-smooth is always a good thing, assuming there aren't regressions in responsiveness. It sounds like we've addressed the touch responsiveness issues and skate "slowness" issues by using a new algorithm, so I don't think I have more to contribute from UX side. At this point it seems to be all about the numbers.

Assuming GFX is on board, we should probably take the normal approach of "land it -> file bugs against regressions", rather than bottlenecking on UX. If we find regressions, we can open a bug.
Assignee: gbrander → mchang
Just to be clear, I have been dogfooding this for a few days, and the jank I talked about in comment 6 still does show up from time to time. And it is definitely more jank than we experience with this feature turned off. I will try to get a profile next time it happens if I am in front of a computer, but I highly recommend someone else besides me dogfood before landing. I'm using the Tako.
Let's not change the pref until we sort out what Michael mentions in comment 6 and comment 12.  Mason, you've done some testing on tako, does that mean you have access to one and a local build you can make?
I don't have a tako. The only reason I was able to test on a tako was because Michael was in the office with a tako and I noticed his scrolling was janky so I wanted to test this on his device. Michael also said that the tako is still pretty buggy. For example, sometimes the display won't come on for a couple of hours. If the vsync events aren't reliably coming in, we'll be janky. I'm going to dogfood on a flame for a while and see if I can reproduce. Gordon, can you also see if you can reproduce what Michael says in your testing? Thanks!
Going to test on :

Gaia: e98b6c9a2ee9b2af4707f6501b7f31f37557e719
Gecko: 205307:56cba2986c61
I noticed a pretty big difference on a flame while scrolling my email inbox. Gordon, can you see any visual difference with your email?
Flags: needinfo?(gbrander)
After lots of scrolling and playing with it for a couple of hours, I got some about-memory reports of master versus with this patch on. I didn't see any memory leaks and I haven't seen any staggering jank that I haven't seen on master. Will continue evaluating over the week.
I hit my first large jank. It occurs after the screen has been off for a while, usually about 2-3 hours. When I turn on the device and unlock, the scrolling immediately after is very janky. I also see lots of checkerboarding, low res tiling, and overall system unresponsiveness. After a couple of seconds, everything smoothes out again. After talking with Michael Henretty, he said he noticed the same thing on a taco and that scrolling also usually smoothes out.

This makes sense from the algorithm's point of view. We smooth out the touch input based on a consistent vsync event. Sometimes we will extrapolate, sometimes we will interpolate. If the main thread is busy and we get a touch event, we will resample based on the last two touch events. Since the last two touch events are added to the resampler on the android input thread, the touch coordinate difference between the last touch event APZ has processed and the last touch event the android input thread has seen will be large, creating very visible jank. Without touch resampling, we will dispatch all touch events in order, creating less visible jank.

The upside to touch resampling is that we do less work in that we process fewer touch events. The downside is that in these bad cases, the jank will be more jarring, but at least we won't keep piling on touch events. I will do more investigation to see if I can consistently recreate this, but the "more jank when the main thread is busy" makes sense. The real way to fix this is get touch events off main thread, which is bug 930939. Personally, I think it's ok if we make the worst case worse in exchange for making the average case better.
See Also: → 1064927
(In reply to Mason Chang [:mchang] from comment #18)
> .... Since the last
> two touch events are added to the resampler on the android input thread, the
> touch coordinate difference between the last touch event APZ has processed
> and the last touch event the android input thread has seen will be large,
> creating very visible jank. Without touch resampling, we will dispatch all
> touch events in order, creating less visible jank.

Sounds like an opportunity to enhance our algorithm to avoid this problem?  I'm not sure if we have, or could have access to all this data in one place, but can we detect this scenario in code and act accordingly, perhaps not doing the event smoothing in some cases?
I am able to somewhat reproduce this. I thought of using my favorite destroy the phone test case, loading nytimes.com. I loaded nytimes.com, let it load, wait for the video to play. This usually takes a minute or so. Everything while scrolling nytimes.com will be janky, checkerboarding / low res tiling, then go to the homescreen. There will be lots of initial jank, especially after the phone has been on for a while. Here is a profile -

https://people.mozilla.org/~bgirard/cleopatra/#report=27689cb1ba1ad5937e3b74e4a44cee97c2bdfbc6

We see lots of 30-50ms garbage collection near the end, and we also see that we don't composite and extra long composites. I'm also worried that we have 78 ms display list rendering times. I'm not convinced this is a resampling issue versus, it's just more apparently that the system is hurting under load.
We're using tons of memory.
Attached file Vide of Extra Jank
Side by side scrolling with touch resampling versus master on the right. We see with touch resampling, we scroll and jank harder with intermittent smoothness. Master has constant jank, with sometimes bigger janks in between.
Blocks: 1069037
No longer blocks: 1069037
Depends on: 1069037
Per last week's meeting, clearing needsinfo https://etherpad.mozilla.org/ux-gfx. Mason wanted more eyes on this. Will give it a spin.
Flags: needinfo?(gbrander)
Depends on: 1073704
Depends on: 1097387
After our discussion, UX says they can see a major difference and approve.
Flags: needinfo?(gbrander)
Yup. No downside, all upside as far as I can tell.
Flags: needinfo?(gbrander)
as comment 24 and comment 25, case resolved.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This actually isn't enabled by default yet, we will soon once we finish enabling parts of silk. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In this case, should we still mark this one as feature-b2g:2.1? Are we targeting to change anything in 2.1?
You're right, this is 2.2 instead.
feature-b2g: 2.1 → 2.2+
Depends on: 1125273
Turn touch resampling on b2g only. Will land next week.
Attachment #8556516 - Flags: review?(bugmail.mozilla)
Comment on attachment 8556516 [details] [diff] [review]
Enable touch resampling on b2g

Review of attachment 8556516 [details] [diff] [review]:
-----------------------------------------------------------------

Is the wait just to let the dust settle a bit?
Attachment #8556516 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Comment on attachment 8556516 [details] [diff] [review]
> Enable touch resampling on b2g
> 
> Review of attachment 8556516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the wait just to let the dust settle a bit?

Yeah, I want to give each pref a week to test before turning on the next one.
Sounds good. FYI I plan on landing 930939 as soon as I get the last r+
Manually tested on today's master, git rev 8e960009be915a67284ba3edd2b8ca67ff601f7c. Everything looking good to enable next week.

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbfa70ab3624
https://hg.mozilla.org/mozilla-central/rev/b79e7b655931
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Uplift to 2.2 next week if everything is stable.
Flags: needinfo?(mchang)
This is part 2, touch resampling of silk.
Flags: needinfo?(npark)
tested with bug 1118530, and other than the jankiness mentioned in comment 20, I don't see any other issues.  But in some instances the browser OOMs, so filed Bug 1129126 for it.  But since this also happens in 2.2, as mchang says, I don't think it's specifically related to this.
Flags: needinfo?(npark)
Depends on: 1129632
Flags: needinfo?(mchang)
Flags: needinfo?(mchang)
Depends on: 1129210
Rebased for mozilla-b2g37_v2_2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Project Silk
User impact if declined: Janky scrolling while tracking a user's finger
Testing completed: Manual, has been enabled on master for 1 week, tested by No-Jun in comment 39. 
Risk to taking this patch (and alternatives if risky): Medium - This changes how we send touch events to send only 1 per vsync interval.
String or UUID changes made by this patch: None

Note: requires bug 1129210, which has b2g-37 approval to land first.
Flags: needinfo?(mchang)
Attachment #8561167 - Flags: approval-mozilla-b2g37?
Attachment #8561167 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: