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.

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

Pointer event resampler (#41118) #60558

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

dreveman
Copy link
Contributor

@dreveman dreveman commented Jun 29, 2020

Pointer event resampler (#41118)

This introduces the PointerEventResampler class that contains
the core logic needed to resample pointer events. This can be used
to get smoother event processing at the cos of some added latency.
Devices with low frequency sensors or when the frequency is not a
multiple of the display frequency (e.g., 120Hz input and 90Hz
display) benefit from this.

The caller is responsible for deciding what sampling offset to use
and what frequency to sample at. If touch events arrive at 65 Hz,
then it's typically good to be using a sampling offset that is
current time - 22 ms for smooth touch event delivery.

This change is also using the PointerEventResampler class to
add resampling support to GestureBinding. Resampling is disabled
by default.

This can be used to fix: #41118

@dreveman dreveman requested review from CareF and liyuqian June 29, 2020 21:47
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 29, 2020
@dreveman dreveman force-pushed the pointer-data-resampler branch 3 times, most recently from 2113a47 to c29a076 Compare July 1, 2020 13:18
@dreveman
Copy link
Contributor Author

dreveman commented Jul 1, 2020

PTAL

@goderbauer
Copy link
Member

nit: Can you please add a short description to the PR describing what this does and what it is for? Also, are there any issues associated with this? If so, please link them from the PR description.

@dreveman
Copy link
Contributor Author

dreveman commented Jul 1, 2020

nit: Can you please add a short description to the PR describing what this does and what it is for? Also, are there any issues associated with this? If so, please link them from the PR description.

Done. Sorry for the lack of explanation earlier. We're planning to have this land together with a potential blog post that explains it in more detail but please let me know what more documentation would be useful in code comments and PR description as reading the post should not be necessary to understand this.

@Piinks Piinks added f: scrolling Viewports, list views, slivers, etc. platform-android Android applications specifically labels Jul 6, 2020
@dreveman dreveman force-pushed the pointer-data-resampler branch 2 times, most recently from 8e967bf to 41db16a Compare July 9, 2020 09:06
@CareF
Copy link
Contributor

CareF commented Jul 10, 2020

I don't mean any review comment or suggestion for modification, but I want to point out the thing I wrote in #60796 (comment) .
This resampling happens in the PointerData level, while our unit test framework flutter_test injects the driving input in the PointerEvent level. So if this is integrated into the framework in the future in someway (like become a on/off flag in the framework), rather than the current use case as replacing the binding in the app, the instance of PointerDataResampler in the framework wouldn't be able to be tested using WidgetTester for integration test because anything from PointerDataResampler would look like device input rather than test input and will be ignored.

@goderbauer
Copy link
Member

(PR triage) @dreveman @liyuqian Are there still plans for this PR? It has been in-active for some weeks now...

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

The algorithm and the unit tests in this PR looks good. Here are a few more things to consider in my opinion in order to make this out of draft and ready to merge:

  1. Use this in GestureBinding and expose a boolean flag to turn this resampling on or off. In the beginning, we'll make it default to off and only enable it in testing. Once we're confident with those tests, turn it to on by default. The use of resampling probably would happen inside GestureBinding._handlePointerDataPacket where PointerData is converted to PointerEvent and added to the queue.

  2. Either in GestureBinding or PointerDataResampler , document why someone would want to resample events, and why someone would not want to resample events.

  3. Decide whether we want to resample at PointerData level or PointerEvent level. According to previous discussion, as long as we're only exposing this resampling as a global flag instead of a widget-level flag, this shouldn't matter much. The small advantage of PointerEvent is a closer integration with flutter_test package, and it's possible for us to enable widget-level resampling in the future. The small advantage of PointerData is that it's closer to the platform implementation (e.g., Fuchsia), it has a simpler class inheritance structure (no subclass such as PointerAddEvent, PointerRemoveEvent, ...), and it may allow us to test lower level part of the system that's inaccessible to PointerEvent tests.

@goderbauer
Copy link
Member

(PR triage) @dreveman @liyuqian Are there still plans for this PR? It has been in-active for some weeks now...

@liyuqian
Copy link
Contributor

liyuqian commented Aug 5, 2020

@dreveman told us this Monday that he's working on an updated resampler for PointerEvent instead of PointerData. The new work should be available soon.

@dreveman dreveman changed the title Pointer data resampler (#41118) Pointer event resampler (#41118) Aug 7, 2020
@dreveman dreveman force-pushed the pointer-data-resampler branch 2 times, most recently from 9412299 to 2d8352d Compare August 21, 2020 01:50
Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

LGTM

@dreveman dreveman force-pushed the pointer-data-resampler branch 2 times, most recently from a2ae63d to d0ecda2 Compare August 21, 2020 12:19
@CareF
Copy link
Contributor

CareF commented Aug 21, 2020

I tried this flag on https://github.com/flutter/flutter/tree/master/dev/benchmarks/complex_layout
with changing the main() in https://github.com/flutter/flutter/blob/master/dev/benchmarks/complex_layout/lib/main.dart to

import 'package:flutter/gestures.dart';

void main() {
  runApp(ComplexLayoutApp());
  GestureBinding.instance.resamplingEnabled = true;
}

It failed and this is what I got:

E/flutter (15136): [ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/gestures/binding.dart': Failed assertion: line 56 pos 16: 'event.timeStamp <= (scheduler?.currentSystemFrameTimeStamp ?? Duration.zero)': is not true.
E/flutter (15136): #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:46:39)
E/flutter (15136): #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:36:5)
E/flutter (15136): #2      _Resampler.addOrDispatchAll (package:flutter/src/gestures/binding.dart:56:16)
E/flutter (15136): #3      GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:207:18)
E/flutter (15136): #4      GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:190:7)
E/flutter (15136): #5      _rootRunUnary (dart:async/zone.dart:1206:13)
E/flutter (15136): #6      _CustomZone.runUnary (dart:async/zone.dart:1100:19)
E/flutter (15136): #7      _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
E/flutter (15136): #8      _invoke1 (dart:ui/hooks.dart:265:10)
E/flutter (15136): #9      _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)
E/flutter (15136): 
I/chatty  (15136): uid=10643(com.example.complex_layout) 1.ui identical 84 lines
E/flutter (15136): [ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/gestures/binding.dart': Failed assertion: line 56 pos 16: 'event.timeStamp <= (scheduler?.currentSystemFrameTimeStamp ?? Duration.zero)': is not true.
E/flutter (15136): #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:46:39)
E/flutter (15136): #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:36:5)
E/flutter (15136): #2      _Resampler.addOrDispatchAll (package:flutter/src/gestures/binding.dart:56:16)
E/flutter (15136): #3      GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:207:18)
E/flutter (15136): #4      GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:190:7)
E/flutter (15136): #5      _rootRunUnary (dart:async/zone.dart:1206:13)
E/flutter (15136): #6      _CustomZone.runUnary (dart:async/zone.dart:1100:19)
E/flutter (15136): #7      _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
E/flutter (15136): #8      _invoke1 (dart:ui/hooks.dart:265:10)
E/flutter (15136): #9      _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)
E/flutter (15136): 

Did I misunderstood how it should be used?

@dreveman
Copy link
Contributor Author

I tried this flag on https://github.com/flutter/flutter/tree/master/dev/benchmarks/complex_layout
with changing the main() in https://github.com/flutter/flutter/blob/master/dev/benchmarks/complex_layout/lib/main.dart to

import 'package:flutter/gestures.dart';

void main() {
  runApp(ComplexLayoutApp());
  GestureBinding.instance.resamplingEnabled = true;
}

It failed and this is what I got:

E/flutter (15136): [ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/gestures/binding.dart': Failed assertion: line 56 pos 16: 'event.timeStamp <= (scheduler?.currentSystemFrameTimeStamp ?? Duration.zero)': is not true.
E/flutter (15136): #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:46:39)
E/flutter (15136): #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:36:5)
E/flutter (15136): #2      _Resampler.addOrDispatchAll (package:flutter/src/gestures/binding.dart:56:16)
E/flutter (15136): #3      GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:207:18)
E/flutter (15136): #4      GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:190:7)
E/flutter (15136): #5      _rootRunUnary (dart:async/zone.dart:1206:13)
E/flutter (15136): #6      _CustomZone.runUnary (dart:async/zone.dart:1100:19)
E/flutter (15136): #7      _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
E/flutter (15136): #8      _invoke1 (dart:ui/hooks.dart:265:10)
E/flutter (15136): #9      _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)
E/flutter (15136): 
I/chatty  (15136): uid=10643(com.example.complex_layout) 1.ui identical 84 lines
E/flutter (15136): [ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/gestures/binding.dart': Failed assertion: line 56 pos 16: 'event.timeStamp <= (scheduler?.currentSystemFrameTimeStamp ?? Duration.zero)': is not true.
E/flutter (15136): #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:46:39)
E/flutter (15136): #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:36:5)
E/flutter (15136): #2      _Resampler.addOrDispatchAll (package:flutter/src/gestures/binding.dart:56:16)
E/flutter (15136): #3      GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:207:18)
E/flutter (15136): #4      GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:190:7)
E/flutter (15136): #5      _rootRunUnary (dart:async/zone.dart:1206:13)
E/flutter (15136): #6      _CustomZone.runUnary (dart:async/zone.dart:1100:19)
E/flutter (15136): #7      _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
E/flutter (15136): #8      _invoke1 (dart:ui/hooks.dart:265:10)
E/flutter (15136): #9      _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)
E/flutter (15136): 

Did I misunderstood how it should be used?

What platform is this? Looks like either currentSystemFrameTimeStamp is not set currently or event timestamps are not set correctly.

@CareF
Copy link
Contributor

CareF commented Aug 21, 2020

What platform is this? Looks like either currentSystemFrameTimeStamp is not set currently or event timestamps are not set correctly.

I tested it on a Pixel 4, with flutter run (debug mode). If I do similar thing on a Fuchsia device should it work?

@CareF
Copy link
Contributor

CareF commented Aug 21, 2020

FYI, if I add

        print(event.timeStamp);
        print(scheduler?.currentSystemFrameTimeStamp);

before that assert, I get:

I/flutter (15686): 413:20:24.378000
I/flutter (15686): 413:20:23.399630

which looks kind of reasonable numbers but for the time difference. If I wait longer time to touch the screen after the app starts, I get larger time difference. It seems that currentSystemFrameTimeStamp is not for the coming frame but the last frame.

@CareF
Copy link
Contributor

CareF commented Aug 21, 2020

If I also add print(scheduler?.currentFrameTimeStamp);, I get

[ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/scheduler/binding.dart': Failed assertion: line 932 pos 12: '_currentFrameTimeStamp != null': is not true.

I guess the timestamp is not yet initialized to the value we expect.

According to https://api.flutter.dev/flutter/scheduler/SchedulerBinding/currentFrameTimeStamp.html I believe that's because we are not using the timestamp within a frame scheduling process. And currentSystemFrameTimeStamp is NOT reset to null which, if that value is going to be documented something other than "this is a more or less arbitrary value", means there's a bug for this.

Comment out this assert will avoid the error message but also blocked all scroll gesture. Tap still works.

@dreveman
Copy link
Contributor Author

currentSystemFrameTimeStamp

This resampling logic depends on currentSystemFrameTimeStamp being a valid time stamp for the expected presentation of the next frame. Hence, always in the future. That's what it is on Fuchsia. We can adjust this logic to expect something else on a different platform but ideally we'd instead make currentSystemFrameTimeStamp the same across all platforms.

@dreveman dreveman force-pushed the pointer-data-resampler branch 2 times, most recently from 6a63be1 to c110ad6 Compare August 22, 2020 21:00
@dreveman
Copy link
Contributor Author

I tried this flag on https://github.com/flutter/flutter/tree/master/dev/benchmarks/complex_layout
with changing the main() in https://github.com/flutter/flutter/blob/master/dev/benchmarks/complex_layout/lib/main.dart to

import 'package:flutter/gestures.dart';

void main() {
  runApp(ComplexLayoutApp());
  GestureBinding.instance.resamplingEnabled = true;
}

It failed and this is what I got:

E/flutter (15136): [ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/gestures/binding.dart': Failed assertion: line 56 pos 16: 'event.timeStamp <= (scheduler?.currentSystemFrameTimeStamp ?? Duration.zero)': is not true.
E/flutter (15136): #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:46:39)
E/flutter (15136): #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:36:5)
E/flutter (15136): #2      _Resampler.addOrDispatchAll (package:flutter/src/gestures/binding.dart:56:16)
E/flutter (15136): #3      GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:207:18)
E/flutter (15136): #4      GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:190:7)
E/flutter (15136): #5      _rootRunUnary (dart:async/zone.dart:1206:13)
E/flutter (15136): #6      _CustomZone.runUnary (dart:async/zone.dart:1100:19)
E/flutter (15136): #7      _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
E/flutter (15136): #8      _invoke1 (dart:ui/hooks.dart:265:10)
E/flutter (15136): #9      _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)
E/flutter (15136): 
I/chatty  (15136): uid=10643(com.example.complex_layout) 1.ui identical 84 lines
E/flutter (15136): [ERROR:flutter/lib/ui/ui_dart_state.cc(171)] Unhandled Exception: 'package:flutter/src/gestures/binding.dart': Failed assertion: line 56 pos 16: 'event.timeStamp <= (scheduler?.currentSystemFrameTimeStamp ?? Duration.zero)': is not true.
E/flutter (15136): #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:46:39)
E/flutter (15136): #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:36:5)
E/flutter (15136): #2      _Resampler.addOrDispatchAll (package:flutter/src/gestures/binding.dart:56:16)
E/flutter (15136): #3      GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:207:18)
E/flutter (15136): #4      GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:190:7)
E/flutter (15136): #5      _rootRunUnary (dart:async/zone.dart:1206:13)
E/flutter (15136): #6      _CustomZone.runUnary (dart:async/zone.dart:1100:19)
E/flutter (15136): #7      _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
E/flutter (15136): #8      _invoke1 (dart:ui/hooks.dart:265:10)
E/flutter (15136): #9      _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)
E/flutter (15136): 

Did I misunderstood how it should be used?

This should work fine now. I found a bug in the copyWith logic that caused down/move/up events to not be produced correctly. Added some tests for this.

Latest version of this change also contains a debug flag that is used to determine if resampling works correctly on a device. I've verified that it works correctly on a Pixel 3 device.

Copy link
Contributor

@CareF CareF left a comment

Choose a reason for hiding this comment

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

LGTM. I wrote a unit test (or in this context, more close to an integration test?) for the resampling performing on widgets, by tracking the events from Listener, which should be a natural unit test for the sampling working with widgets. I'll file another PR for that after this lands and hope you can review that.

This introduces the PointerEventResampler class that contains
the core logic needed to resample pointer events. This can be used
to get smoother event processing at the cos of some added latency.
Devices with low frequency sensors or when the frequency is not a
multiple of the display frequency (e.g., 120Hz input and 90Hz
display) benefit from this.

The caller is responsible for deciding what sampling offset to use
and what frequency to sample at. If touch events arrive at 65 Hz,
then it's typically good to be using a sampling offset that is
current time - 22 ms for smooth touch event delivery.

This change is also using the PointerEventResampler class to
add resampling support to GestureBinding. Resampling is disabled
by default.

This can be used to fix: flutter#41118

final PointerEventResampler resampler = _resamplers.putIfAbsent(
event.device,
() => PointerEventResampler(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it true that when this constructor is called (i.e., event.device key is absent in _resamplers), event can only be PointerAddedEvent or PointerDownEvent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that would only be true if resampling was enabled before the first pointer event. If you enable resampling after first receiving a down event then we'll need to create the resampler here when processing a move event.

@fluttergithubbot fluttergithubbot merged commit 02612bf into flutter:master Aug 24, 2020
smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

if (resamplingEnabled) {
_resampler.addOrDispatchAll(_pendingPointerEvents);
_resampler.sample(samplingOffset);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that, when the pending event queue in the resampler is not empty, but there are no more future inputs, these pending events will stay in the queue and will not be processed, since _flushPointerEventQueue is not called?
I realize this when I'm trying to move resampler to handlePointerEvent.
cc @liyuqian

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the _Resampler will schedule _handleSampleTimeChanged after frame callback which will call _flushPointerEventQueue. See: https://github.com/flutter/flutter/pull/60558/files/b7802f69f91e1b5466d6b5b58e98193160964ca5#diff-6305361e0c7677f0eebfb05dfcbe0336

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-android Android applications specifically platform-fuchsia Fuchsia code specifically platform-ios iOS applications specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resample input events due to irregular events delivery or VSYNC and input frequency mismatch
7 participants