Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Increase Win32 trackpad polling rate, fix repeated swipes issue #37154

Merged
merged 3 commits into from
Nov 4, 2022
Merged

Increase Win32 trackpad polling rate, fix repeated swipes issue #37154

merged 3 commits into from
Nov 4, 2022

Conversation

moffatman
Copy link
Contributor

Increase polling rate from 60 Hz to 240 Hz or 4 times the monitor refresh rate. It seems like the polling rate does not match 1:1 with the returned updates, eventually leading to scrolls with missing framework momentum. This was because the gap between updates was too large (above 40 ms), so the velocity tracker in the framework returned an estimate of zero.

Also fix an issue introduced in #34452, where the transition to using inertia means that repeated swipes had no opportunity to reset the transform matrix. Now pan zoom updates will compare with the gesture data at pan zoom start time. Eventually, once swipes stop, the matrix will be reset to zero.

Fixes flutter/flutter#114082

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

dkwingsmt
dkwingsmt reviewed Nov 2, 2022
View reviewed changes
shell/platform/windows/window.h Outdated
@@ -294,7 +294,7 @@ class Window : public KeyboardManager::WindowDelegate {
const static int kDirectManipulationTimer = 1;

// Frequency (Hz) to poll for DirectManipulation updates.
int directManipulationPollingRate_ = 60;
int directManipulationPollingRate_ = 240;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining this magic value?

dkwingsmt
dkwingsmt reviewed Nov 2, 2022
View reviewed changes
shell/platform/windows/window.cc Outdated
@@ -110,7 +110,7 @@ void Window::InitializeChild(const char* title,
ZeroMemory(&dmi, sizeof(dmi));
dmi.dmSize = sizeof(dmi);
if (EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &dmi)) {
directManipulationPollingRate_ = dmi.dmDisplayFrequency;
directManipulationPollingRate_ = 4 * dmi.dmDisplayFrequency;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 240Hz and this 4 * frequency, are we assuming a 60Hz of something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the display is 60 Hz in case we aren't able to get the actual value. When calling Update() at 60 Hz I have seen 30 Hz or worse actual updates. Not sure what is the root cause, but when polling rate is increased the experience feels a lot better. At worst I saw 200ms between events. I did x4 to be safe as the velocity tracker which controls momentum only looks at the latest 100ms of events.

I didn't spend much time on optimization here, it may be that with further testing 3 * displayFrequency might work just as well.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Nov 2, 2022 •
edited
Loading

I'm ok with the logic change.

But I'm not so sure about the polling frequency change. If it resolves an issue that observable from a hardware device, we can land it, but I think it's better to split it into a separate PR, with more explanation (the issue it resolves, how to reproduce and verify the fix), as well as a test exempt if necessary.

@moffatman
Copy link
Contributor Author

OK I can split it out and also collect some data as to update polling rate vs actual update rate

@moffatman
Copy link
Contributor Author

Looked into it and I had 2 mistakes in my SetTimer code. The first is that SetTimer is already periodic, so rescheduling it isn't needed (although I don't think this had any ill effects). The second is that SetTimer is not very precise, it is in some way tied to the other window event loop messages. Reducing the requested interval from 16 ms to 14 ms led to updates at the desired 16 ms. And the minimum possible interval is 10 ms, so no need to try and get 120 Hz callbacks.

image

image

image

dkwingsmt
dkwingsmt approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moffatman moffatman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 4, 2022
@auto-submit auto-submit bot merged commit af1a2da into flutter:main Nov 4, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 4, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2022
@engine-flutter-autoroll
af1a2da13 Increase Win32 trackpad polling rate, fix repeated swipes i…
f189ee6 
…ssue (flutter/engine#37154)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 5, 2022
@engine-flutter-autoroll
Roll Flutter Engine from 0933ca485a07 to 28e1ea0cc5b1 (4 revisions) (#…
96f9ca8 
…114720)

* d413cc511 Add unreachable for vulkan switches (flutter/engine#37324)

* e32bff47e Copy Windows gen_snapshot to a standard location (flutter/engine#37318)

* af1a2da13 Increase Win32 trackpad polling rate, fix repeated swipes issue (flutter/engine#37154)

* 28e1ea0cc Roll Fuchsia Mac SDK from JKfnEvEVIL_Cg3_9f... to jAKH68TYoKUA5HNS2... (flutter/engine#37325)
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@moffatman @schwa423
Increase Win32 trackpad polling rate, fix repeated swipes issue (flut…
6d49901 
…ter#37154)

* Windows trackpad improvements

* Address feedback, remove dynamic polling rate

* Adjust comment
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
@engine-flutter-autoroll @shogohida
Roll Flutter Engine from 0933ca485a07 to 28e1ea0cc5b1 (4 revisions) (f…
77363ed 
…lutter#114720)

* d413cc511 Add unreachable for vulkan switches (flutter/engine#37324)

* e32bff47e Copy Windows gen_snapshot to a standard location (flutter/engine#37318)

* af1a2da13 Increase Win32 trackpad polling rate, fix repeated swipes issue (flutter/engine#37154)

* 28e1ea0cc Roll Fuchsia Mac SDK from JKfnEvEVIL_Cg3_9f... to jAKH68TYoKUA5HNS2... (flutter/engine#37325)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Flutter Engine from 0933ca485a07 to 28e1ea0cc5b1 (4 revisions) (f…
217d4eb 
…lutter#114720)

* d413cc511 Add unreachable for vulkan switches (flutter/engine#37324)

* e32bff47e Copy Windows gen_snapshot to a standard location (flutter/engine#37318)

* af1a2da13 Increase Win32 trackpad polling rate, fix repeated swipes issue (flutter/engine#37154)

* 28e1ea0cc Roll Fuchsia Mac SDK from JKfnEvEVIL_Cg3_9f... to jAKH68TYoKUA5HNS2... (flutter/engine#37325)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loic-sharma loic-sharma loic-sharma left review comments

@dkwingsmt dkwingsmt dkwingsmt approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Windows] Trackpad scroll is inconsistent, abruptly stops or scrolls in wrong direction
3 participants
@moffatman @dkwingsmt @loic-sharma