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

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

Merged
merged 6 commits into from Sep 30, 2019

Conversation

@liyuqian
Copy link
Contributor

liyuqian commented Sep 22, 2019

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
@googlebot googlebot added the cla: yes label Sep 22, 2019
@liyuqian liyuqian force-pushed the liyuqian:reland branch from 79cc56b to a6f62c7 Sep 22, 2019
@liyuqian liyuqian requested review from chinmaygarde and cbracken Sep 23, 2019
@liyuqian liyuqian requested a review from gaaclarke Sep 24, 2019
Copy link
Contributor

gaaclarke left a comment

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.

@@ -88,6 +88,12 @@ sk_sp<GrContext> PlatformView::CreateResourceContext() const {

void PlatformView::ReleaseResourceContext() const {}

PointerDataDispatcherMaker PlatformView::GetDispatcherMaker() {

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 26, 2019

Contributor

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

/// (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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

s/send/sends

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

@@ -52,6 +52,8 @@ class Animator final {

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

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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

Add docstring.

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

@@ -36,6 +36,7 @@ static constexpr char kSettingsChannel[] = "flutter/settings";
static constexpr char kIsolateChannel[] = "flutter/isolate";

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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

I believe you want const here.

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

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

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

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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

Add docstring

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

[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 comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

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

@@ -26,6 +26,8 @@ class VsyncWaiter : public std::enable_shared_from_this<VsyncWaiter> {

void AsyncWaitForVsync(Callback callback);

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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

docstring

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

@@ -46,48 +46,87 @@ void VsyncWaiter::AsyncWaitForVsync(Callback callback) {
return;
}
callback_ = std::move(callback);
if (secondary_callback_) {
// Return directly as `AwaitVSync` is already called by

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

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

// Return directly as `AwaitVSync` is already called by
// `AsyncWaitForVsync`.
return;
}
}
AwaitVSync();

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done. Added a trace event similar to MultipleCallsToVsyncInFrameInterval.

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 26, 2019

Contributor

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 26, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 25, 2019

Contributor

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 26, 2019

Author Contributor

Done. Added a trace event similar to MultipleCallsToVsyncInFrameInterval.

Copy link
Contributor Author

liyuqian left a comment

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

@@ -36,6 +36,7 @@ static constexpr char kSettingsChannel[] = "flutter/settings";
static constexpr char kIsolateChannel[] = "flutter/isolate";

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

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

@@ -52,6 +52,8 @@ class Animator final {

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

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

@@ -88,6 +88,12 @@ sk_sp<GrContext> PlatformView::CreateResourceContext() const {

void PlatformView::ReleaseResourceContext() const {}

PointerDataDispatcherMaker PlatformView::GetDispatcherMaker() {

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

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.

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

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

// 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_;

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

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.

/// (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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

// Return directly as `AwaitVSync` is already called by
// `AsyncWaitForVsync`.
return;
}
}
AwaitVSync();

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done. Added a trace event similar to MultipleCallsToVsyncInFrameInterval.

@@ -26,6 +26,8 @@ class VsyncWaiter : public std::enable_shared_from_this<VsyncWaiter> {

void AsyncWaitForVsync(Callback callback);

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

This comment has been minimized.

Copy link
@liyuqian

liyuqian Sep 25, 2019

Author Contributor

Done.

@liyuqian

This comment has been minimized.

Copy link
Contributor Author

liyuqian commented Sep 26, 2019

@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());
+    });
+  });
 }
Copy link
Member

chinmaygarde left a comment

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

@@ -52,6 +52,8 @@ class Animator final {

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

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

@@ -52,6 +52,8 @@ class Animator final {

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

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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.

@@ -244,4 +244,8 @@ void Animator::AwaitVSync() {
delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_);
}

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

@@ -234,6 +236,12 @@ class Engine final : public RuntimeDelegate {
/// 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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

class PointerDataDispatcher;

//------------------------------------------------------------------------------
/// The `Engine` pointer data dispatcher that forwards the packet received from

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

This is great. Thank for explaining it.

/// 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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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.

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

@@ -52,6 +52,8 @@ class Animator final {

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

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Sep 26, 2019

Member

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

};

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

This comment has been minimized.

Copy link
@gaaclarke

gaaclarke Sep 26, 2019

Contributor

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

Copy link
Contributor

gaaclarke left a comment

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

@liyuqian liyuqian force-pushed the liyuqian:reland branch from bd95d68 to 2499198 Sep 27, 2019
@liyuqian liyuqian requested a review from chinmaygarde Sep 27, 2019
@liyuqian liyuqian force-pushed the liyuqian:reland branch from 2499198 to 11c5680 Sep 27, 2019
@liyuqian liyuqian merged commit 9675ca2 into flutter:master Sep 30, 2019
15 checks passed
15 checks passed
WIP Ready for review
Details
build_and_test_android_unopt_debug Task Summary
Details
build_and_test_android_unopt_debug
Details
build_and_test_linux_unopt_debug Task Summary
Details
build_and_test_linux_unopt_debug
Details
build_fuchsia_artifacts Task Summary
Details
build_fuchsia_artifacts
Details
build_windows_opt_debug Task Summary
Details
build_windows_opt_debug
Details
build_windows_unopt_debug Task Summary
Details
build_windows_unopt_debug
Details
cla/google All necessary CLAs are signed
format_and_dart_test Task Summary
Details
format_and_dart_test
Details
luci-engine
Details
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 30, 2019
git@github.com:flutter/engine.git/compare/1f454c75330c...9675ca2

git log 1f454c7..9675ca2 --no-merges --oneline
2019-09-30 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (#12280)" (flutter/engine#12385)
2019-09-30 gspencergoog@users.noreply.github.com Add missing flag for embedder. (flutter/engine#12700)
2019-09-30 lu.zuccarini@gmail.com 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 aaclarke@google.com 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
kangwang1988 added a commit to alibaba-flutter/flutter that referenced this pull request Oct 8, 2019
git@github.com:flutter/engine.git/compare/1f454c75330c...9675ca2

git log 1f454c7..9675ca2 --no-merges --oneline
2019-09-30 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (flutter#12280)" (flutter/engine#12385)
2019-09-30 gspencergoog@users.noreply.github.com Add missing flag for embedder. (flutter/engine#12700)
2019-09-30 lu.zuccarini@gmail.com 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 aaclarke@google.com 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
Inconnu08 added a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
git@github.com:flutter/engine.git/compare/1f454c75330c...9675ca2

git log 1f454c7..9675ca2 --no-merges --oneline
2019-09-30 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (flutter#12280)" (flutter/engine#12385)
2019-09-30 gspencergoog@users.noreply.github.com Add missing flag for embedder. (flutter/engine#12700)
2019-09-30 lu.zuccarini@gmail.com 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 aaclarke@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.