Reland "Smooth out iOS irregular input events delivery (#12280)" by liyuqian · Pull Request #12385 · flutter/engine

liyuqian · 2019-09-22T05:30:43Z

This reverts commit c2879ca.

Additionally, we fix flutter/flutter#40863 by adding a secondary VSYNC callback.

Unit tests are updated to provide VSYNC mocking and check the fix of flutter/flutter#40863.

The root cause of having flutter/flutter#40863 is the false assumption that each input event must trigger a new frame. That was true in the framework PR flutter/flutter#36616 because the input events there are all scrolling move events. When the PR was ported to the engine, we can no longer distinguish different types of events, and tap events may no longer trigger a new frame.

Therefore, this PR directly hooks into the VsyncWaiter and uses its (newly added) secondary callback to dispatch the pending input event.

It's probably more challenging to write a good unit test for this PR than to actually fix the issue because we have to mock the correct orders of numerous multi-threading events reliably. Therefore, this PR has 3 commits:

39f9ec6 : the revert which probably doesn't need much review
fda31bf : the fix of the issue
a6f62c7 : the unit tests update

This reverts commit c2879ca.

gaaclarke

My only big question is why does the method ScheduleSecondaryCallback cause an AwaitVSync. That doesn't seem right.

For my comments about naming, those are just thoughts and suggestions, there wasn't anything I really felt strongly about.

For my comments about docstrings, you can probably just point them someplace else for more information instead of having to explain SetSecondaryCallback multiple times.

gaaclarke · 2019-09-24T23:52:51Z

shell/common/platform_view.cc


 void PlatformView::ReleaseResourceContext() const {}

+PointerDataDispatcherMaker PlatformView::GetDispatcherMaker() {


Typically something whose job it is is to make something is called a "factory". Are we referring these to these as "makers" elsewhere?

There doesn't seem to be any maker in Flutter, but there are several in Skia. I guess that I didn't make a full factory class just for simplicity (instead of calling factory->Make(...), I'll directly call maker(...).

I think that I can change it to factory for better readability. Although I probably won't do it in this PR to limit the scope because the naming of this Maker happened before this revert/reland.

I was just suggesting a rename of the typedef, not actually making a factory class =)

gaaclarke · 2019-09-24T23:57:51Z

shell/common/pointer_data_dispatcher.h

+///   (2) The `PlatformView` can only be accessed from the PlatformThread while
+///       this class (as owned by engine) can only be accessed in the UI thread.
+///       Hence `PlatformView` creates a `PointerDataDispatchMaker` on the
+///       platform thread, and send it to the UI thread for the final


s/send/sends

gaaclarke · 2019-09-25T00:04:06Z

shell/common/animator.h


  void Render(std::unique_ptr<flutter::LayerTree> layer_tree);

+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback);


Add docstring.

Nit: Using the @see doxygen directive will link the same nicely in generated documentation. For example, https://github.com/flutter/flutter/blob/26465f4c657bc5e3bdbcf3b0b399e6e41622a185/packages/flutter_tools/lib/src/macos/xcode.dart

Can you make the docstring more descriptive please? It mostly just repeats the method name. I have no idea why this is necessary just based on reading that.

I read the docstring later. Can you also repeat the same thing here for clarity without having to jump through links.

gaaclarke · 2019-09-25T00:05:36Z

shell/common/engine.cc

 static constexpr char kIsolateChannel[] = "flutter/isolate";

 Engine::Engine(Delegate& delegate,
+               PointerDataDispatcherMaker& dispatcher_maker,


I believe you want const here.

Sure, done. (Although this has nothing to do with the fix for this revert/reland.)

gaaclarke · 2019-09-25T00:08:45Z

shell/common/engine.h

+  void DoDispatchPacket(std::unique_ptr<PointerDataPacket> packet,
+                        uint64_t trace_flow_id) override;
+
+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback) override;


Add docstring

gaaclarke · 2019-09-25T00:35:39Z

shell/common/shell.cc

-      [engine = engine_->GetWeakPtr(), packet = std::move(packet),
-       flow_id = next_pointer_flow_id_] {
+  task_runners_.GetUITaskRunner()->PostTask(
+      fml::MakeCopyable([engine = weak_engine_, packet = std::move(packet),


This this looks good to me. It's easy to get wrong so just pointing it out.

gaaclarke · 2019-09-25T00:38:17Z

shell/common/vsync_waiter.h


  void AsyncWaitForVsync(Callback callback);

+  void ScheduleSecondaryCallback(std::function<void()> callback);


gaaclarke · 2019-09-25T00:40:36Z

shell/common/vsync_waiter.cc

    }
    callback_ = std::move(callback);
+    if (secondary_callback_) {
+      // Return directly as `AwaitVSync` is already called by


This is unfortunate. I wonder if there is some way we can enforce it gets called at least once either way.

gaaclarke · 2019-09-25T00:45:42Z

shell/common/vsync_waiter.cc

+      return;
+    }
  }
  AwaitVSync();


Why would scheduling a callback ever cause the caller to wait for a vsync?

Done. Added a trace event similar to MultipleCallsToVsyncInFrameInterval.

I would like to talk about this offline, too. I don't think I communicated my point well.

Oops, I just realized that I replied to the wrong comment... Glad that we talked offline.

gaaclarke · 2019-09-25T00:46:30Z

shell/common/vsync_waiter.cc

+    std::scoped_lock lock(callback_mutex_);
+    if (secondary_callback_) {
+      // Multiple schedules must result in a single callback per frame interval.
+      return;


Shouldn't this at least print out a warning that the callback is getting thrown away?

Done. Added a trace event similar to MultipleCallsToVsyncInFrameInterval.

liyuqian

Thank you very much for the detailed comments! I've fixed the easy things and let's discuss more subtle issues offline.

liyuqian · 2019-09-25T18:36:54Z

shell/common/engine.cc

 static constexpr char kIsolateChannel[] = "flutter/isolate";

 Engine::Engine(Delegate& delegate,
+               PointerDataDispatcherMaker& dispatcher_maker,


Sure, done. (Although this has nothing to do with the fix for this revert/reland.)

liyuqian · 2019-09-25T21:29:55Z

shell/common/animator.h


  void Render(std::unique_ptr<flutter::LayerTree> layer_tree);

+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback);


liyuqian · 2019-09-25T21:31:13Z

shell/common/platform_view.cc


 void PlatformView::ReleaseResourceContext() const {}

+PointerDataDispatcherMaker PlatformView::GetDispatcherMaker() {


There doesn't seem to be any maker in Flutter, but there are several in Skia. I guess that I didn't make a full factory class just for simplicity (instead of calling factory->Make(...), I'll directly call maker(...).

I think that I can change it to factory for better readability. Although I probably won't do it in this PR to limit the scope because the naming of this Maker happened before this revert/reland.

liyuqian · 2019-09-25T21:33:47Z

shell/common/engine.h

+  void DoDispatchPacket(std::unique_ptr<PointerDataPacket> packet,
+                        uint64_t trace_flow_id) override;
+
+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback) override;


liyuqian · 2019-09-25T21:38:21Z

shell/common/engine.h

+  // The pointer_data_dispatcher_ depends on animator_ and runtime_controller_.
+  // So it should be defined after them to ensure that pointer_data_dispatcher_
+  // is destructed first.
+  std::unique_ptr<PointerDataDispatcher> pointer_data_dispatcher_;


Do you mean to use WeakPtr<Engine::Delegate> instead of Engine::Delegate&? I think we can do that in a new PR since this is not related to the revert/reland and I want the scope of this PR to be limited to that only.

liyuqian · 2019-09-25T23:19:44Z

shell/common/pointer_data_dispatcher.h

+///   (2) The `PlatformView` can only be accessed from the PlatformThread while
+///       this class (as owned by engine) can only be accessed in the UI thread.
+///       Hence `PlatformView` creates a `PointerDataDispatchMaker` on the
+///       platform thread, and send it to the UI thread for the final


liyuqian · 2019-09-25T23:20:06Z

shell/common/pointer_data_dispatcher.h

+    ///
+    ///           This callback is used to provide the vsync signal needed by
+    ///           `SmoothPointerDataDispatcher`.
+    virtual void ScheduleSecondaryVsyncCallback(


liyuqian · 2019-09-25T23:26:03Z

shell/common/shell_test.cc

+  fml::TaskRunner::RunNowOrPostTask(
+      shell->GetTaskRunners().GetUITaskRunner(),
+      [shell, &latch, &configuration]() {
+        bool restarted = shell->engine_->Restart(std::move(configuration));


liyuqian · 2019-09-25T23:31:08Z

shell/common/vsync_waiter.cc

+      return;
+    }
  }
  AwaitVSync();


Done. Added a trace event similar to MultipleCallsToVsyncInFrameInterval.

liyuqian · 2019-09-25T23:32:28Z

shell/common/vsync_waiter.h


  void AsyncWaitForVsync(Callback callback);

+  void ScheduleSecondaryCallback(std::function<void()> callback);


liyuqian · 2019-09-26T22:29:35Z

@gaaclarke @chinmaygarde : I now realized that my ShellTestVsyncWaiter actually blocks the thread in AwaitVSync (while VsyncWaiterIOS and VsyncWaiterAndroid don't). It's Ok for the unit test, but I wonder if we want to enforce AwaitVSync to never block the current thread, like the following?

 void ShellTestVsyncWaiter::AwaitVSync() {
   FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread());
   auto vsync_future = clock_.NextVSync();
-  vsync_future.wait();
-
-  // Post the `FireCallback` to the Platform thread so earlier Platform tasks
-  // (specifically, the `VSyncFlush` call) will be finished before
-  // `FireCallback` is executed. This is only needed for our unit tests.
-  //
-  // Without this, the repeated VSYNC signals in `VSyncFlush` may start both the
-  // current frame in the UI thread and the next frame in the secondary
-  // callback (both of them are waiting for VSYNCs). That breaks the unit test's
-  // assumption that each frame's VSYNC must be issued by different `VSyncFlush`
-  // call (which reset the `will_draw_new_frame` bit).
-  //
-  // For example, HandlesActualIphoneXsInputEvents will fail without this.
-  task_runners_.GetPlatformTaskRunner()->PostTask(
-      [this]() { FireCallback(fml::TimePoint::Now(), fml::TimePoint::Now()); });
+
+  std::async([&vsync_future, this]() {
+    vsync_future.wait();
+
+    // Post the `FireCallback` to the Platform thread so earlier Platform tasks
+    // (specifically, the `VSyncFlush` call) will be finished before
+    // `FireCallback` is executed. This is only needed for our unit tests.
+    //
+    // Without this, the repeated VSYNC signals in `VSyncFlush` may start both
+    // the current frame in the UI thread and the next frame in the secondary
+    // callback (both of them are waiting for VSYNCs). That breaks the unit
+    // test's assumption that each frame's VSYNC must be issued by different
+    // `VSyncFlush` call (which reset the `will_draw_new_frame` bit).
+    //
+    // For example, HandlesActualIphoneXsInputEvents will fail without this.
+    task_runners_.GetPlatformTaskRunner()->PostTask([this]() {
+      FireCallback(fml::TimePoint::Now(), fml::TimePoint::Now());
+    });
+  });
 }

chinmaygarde

I agree that AwaitVsync should not block. Not sure about the implementation though. Will need to investigate further.

chinmaygarde · 2019-09-26T22:55:32Z

shell/common/animator.h


  void Render(std::unique_ptr<flutter::LayerTree> layer_tree);

+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback);


Nit: Using the @see doxygen directive will link the same nicely in generated documentation. For example, https://github.com/flutter/flutter/blob/26465f4c657bc5e3bdbcf3b0b399e6e41622a185/packages/flutter_tools/lib/src/macos/xcode.dart

chinmaygarde · 2019-09-26T22:57:07Z

shell/common/animator.h


  void Render(std::unique_ptr<flutter::LayerTree> layer_tree);

+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback);


Can you make the docstring more descriptive please? It mostly just repeats the method name. I have no idea why this is necessary just based on reading that.

chinmaygarde · 2019-09-26T22:57:42Z

shell/common/animator.cc

  delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_);
 }

+void Animator::ScheduleSecondaryVsyncCallback(std::function<void()> callback) {


You can replace std::function<void(void)> with fml::closure.

chinmaygarde · 2019-09-26T22:58:51Z

shell/common/engine.h

  ///                                tasks that require access to components
  ///                                that cannot be safely accessed by the
  ///                                engine. This is the shell.
+  /// @param      dispatcher_maker   The `std::function` provided by


"callback" is fine. You don't have to specify the type.

chinmaygarde · 2019-09-26T23:00:12Z

shell/common/pointer_data_dispatcher.h

+class PointerDataDispatcher;
+
+//------------------------------------------------------------------------------
+/// The `Engine` pointer data dispatcher that forwards the packet received from


This is great. Thank for explaining it.

chinmaygarde · 2019-09-26T23:02:26Z

shell/common/pointer_data_dispatcher.h

+/// The dispatcher that filters out irregular input events delivery to provide
+/// a smooth scroll on iPhone X/Xs.
+///
+/// This fixes https://github.com/flutter/flutter/issues/31086.


I don't think this is necessary. You have provided all details necessary in the documentation (if you don't think you have, please elucidate it here). Git archaeologists will be able to get to the bug from the commit anyway.

chinmaygarde · 2019-09-26T23:02:58Z

shell/common/pointer_data_dispatcher.h

+/// See also input_events_unittests.cc where we test all our claims above.
+class SmoothPointerDataDispatcher : public DefaultPointerDataDispatcher {
+ public:
+  SmoothPointerDataDispatcher(Delegate& delegate)


out of line this please. Just like you did with the destructor.

chinmaygarde · 2019-09-26T23:07:54Z

shell/common/animator.h


  void Render(std::unique_ptr<flutter::LayerTree> layer_tree);

+  void ScheduleSecondaryVsyncCallback(std::function<void()> callback);


I read the docstring later. Can you also repeat the same thing here for clarity without having to jump through links.

chinmaygarde · 2019-09-26T23:10:52Z

shell/common/pointer_data_dispatcher.h

+    ///           by `Animator::RequestFrame`).
+    ///
+    ///           Like the callback in `AsyncWaitForVsync`, this callback is
+    ///           only scheduled to be called once, and it's supposed to be


"it will be called on" instead of "supposed to be called"

gaaclarke · 2019-09-26T23:13:57Z

shell/common/pointer_data_dispatcher.h

+};
+
+//------------------------------------------------------------------------------
+/// The dispatcher that filters out irregular input events delivery to provide


"A dispatcher that may temporarily store the last received PointerData for delivery next vsync in order to smooth out the events." ?

gaaclarke

LGTM with the note that I think the doc on SmoothPointerDataDispatcher could be more clear and concise.

…er#12280)" (flutter/engine#12385)

[email protected]:flutter/engine.git/compare/1f454c75330c...9675ca2 git log 1f454c7..9675ca2 --no-merges --oneline 2019-09-30 [email protected] Reland "Smooth out iOS irregular input events delivery (#12280)" (flutter/engine#12385) 2019-09-30 [email protected] Add missing flag for embedder. (flutter/engine#12700) 2019-09-30 [email protected] Add a method to flutter_window_controller to destroy the current window. (flutter/engine#12076) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

[email protected]:flutter/engine.git/compare/1f454c75330c...9675ca2 git log 1f454c7..9675ca2 --no-merges --oneline 2019-09-30 [email protected] Reland "Smooth out iOS irregular input events delivery (flutter#12280)" (flutter/engine#12385) 2019-09-30 [email protected] Add missing flag for embedder. (flutter/engine#12700) 2019-09-30 [email protected] Add a method to flutter_window_controller to destroy the current window. (flutter/engine#12076) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

liyuqian added 2 commits September 20, 2019 15:45

Reland "Smooth out iOS irregular input events delivery (flutter#12280)"

39f9ec6

This reverts commit c2879ca.

Fix by adding a secondary VSYNC callback

fda31bf

googlebot added the cla: yes label Sep 22, 2019

Update unit tests

a6f62c7

liyuqian force-pushed the reland branch from 79cc56b to a6f62c7 Compare September 22, 2019 20:48

liyuqian requested review from cbracken and chinmaygarde September 23, 2019 16:49

Use default vsync waiter in previous tests

cc1f9ed

liyuqian requested a review from gaaclarke September 24, 2019 23:07

gaaclarke reviewed Sep 25, 2019

View reviewed changes

liyuqian commented Sep 25, 2019

View reviewed changes

Fixes for Aaron's comments.

e62f996

chinmaygarde reviewed Sep 26, 2019

View reviewed changes

gaaclarke reviewed Sep 26, 2019

View reviewed changes

gaaclarke approved these changes Sep 26, 2019

View reviewed changes

liyuqian force-pushed the reland branch from bd95d68 to 2499198 Compare September 27, 2019 18:28

liyuqian requested a review from chinmaygarde September 27, 2019 18:30

More fixes for comments

11c5680

liyuqian force-pushed the reland branch from 2499198 to 11c5680 Compare September 27, 2019 19:53

chinmaygarde approved these changes Sep 30, 2019

View reviewed changes

liyuqian merged commit 9675ca2 into flutter:master Sep 30, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2019

9675ca2 Reland "Smooth out iOS irregular input events delivery (flutt…

f46a2db

…er#12280)" (flutter/engine#12385)

engine-flutter-autoroll mentioned this pull request Sep 30, 2019

Roll engine 1f454c75330c..9675ca2f6b00 (3 commits) flutter/flutter#41628

Merged

liyuqian deleted the reland branch April 17, 2020 17:24

KevinHaendel mentioned this pull request Nov 9, 2022

Performance on iPhone XS / iPhone XR significantly low (~20fps instead of 60fps) flutter/flutter#115036

Closed


		void PlatformView::ReleaseResourceContext() const {}

		PointerDataDispatcherMaker PlatformView::GetDispatcherMaker() {


		void Render(std::unique_ptr<flutter::LayerTree> layer_tree);

		void ScheduleSecondaryVsyncCallback(std::function<void()> callback);


		void AsyncWaitForVsync(Callback callback);

		void ScheduleSecondaryCallback(std::function<void()> callback);

Conversation

liyuqian commented Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Sep 26, 2019

Uh oh!

liyuqian commented Sep 22, 2019 •

edited

Loading