From 39f9ec6fddd406bc25ef5aa578074ca990db0316 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 20 Sep 2019 15:43:23 -0700 Subject: [PATCH 1/6] Reland "Smooth out iOS irregular input events delivery (#12280)" This reverts commit c2879cae2ee3707ad07af1118bf4862dc1d82bb7. --- ci/licenses_golden/licenses_flutter | 3 + shell/common/BUILD.gn | 3 + shell/common/engine.cc | 24 +- shell/common/engine.h | 26 +- shell/common/fixtures/shell_test.dart | 11 + shell/common/input_events_unittests.cc | 250 ++++++++++++++++++ shell/common/platform_view.cc | 6 + shell/common/platform_view.h | 89 ++++--- shell/common/pointer_data_dispatcher.cc | 74 ++++++ shell/common/pointer_data_dispatcher.h | 174 ++++++++++++ shell/common/shell.cc | 14 +- shell/common/shell_test.cc | 29 ++ shell/common/shell_test.h | 5 + shell/platform/darwin/ios/platform_view_ios.h | 3 + .../platform/darwin/ios/platform_view_ios.mm | 6 + 15 files changed, 667 insertions(+), 50 deletions(-) create mode 100644 shell/common/input_events_unittests.cc create mode 100644 shell/common/pointer_data_dispatcher.cc create mode 100644 shell/common/pointer_data_dispatcher.h diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 9033f121a69e1..bfe5e9ab5e748 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -486,6 +486,7 @@ FILE: ../../../flutter/shell/common/canvas_spy_unittests.cc FILE: ../../../flutter/shell/common/engine.cc FILE: ../../../flutter/shell/common/engine.h FILE: ../../../flutter/shell/common/fixtures/shell_test.dart +FILE: ../../../flutter/shell/common/input_events_unittests.cc FILE: ../../../flutter/shell/common/isolate_configuration.cc FILE: ../../../flutter/shell/common/isolate_configuration.h FILE: ../../../flutter/shell/common/persistent_cache.cc @@ -495,6 +496,8 @@ FILE: ../../../flutter/shell/common/pipeline.h FILE: ../../../flutter/shell/common/pipeline_unittests.cc FILE: ../../../flutter/shell/common/platform_view.cc FILE: ../../../flutter/shell/common/platform_view.h +FILE: ../../../flutter/shell/common/pointer_data_dispatcher.cc +FILE: ../../../flutter/shell/common/pointer_data_dispatcher.h FILE: ../../../flutter/shell/common/rasterizer.cc FILE: ../../../flutter/shell/common/rasterizer.h FILE: ../../../flutter/shell/common/run_configuration.cc diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index 34ce4a61bc31d..b3745aef2f262 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -74,6 +74,8 @@ source_set("common") { "pipeline.h", "platform_view.cc", "platform_view.h", + "pointer_data_dispatcher.cc", + "pointer_data_dispatcher.h", "rasterizer.cc", "rasterizer.h", "run_configuration.cc", @@ -156,6 +158,7 @@ if (current_toolchain == host_toolchain) { shell_host_executable("shell_unittests") { sources = [ "canvas_spy_unittests.cc", + "input_events_unittests.cc", "pipeline_unittests.cc", "shell_test.cc", "shell_test.h", diff --git a/shell/common/engine.cc b/shell/common/engine.cc index fa3fb65c8172d..18dd07cc82751 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -36,6 +36,7 @@ static constexpr char kSettingsChannel[] = "flutter/settings"; static constexpr char kIsolateChannel[] = "flutter/isolate"; Engine::Engine(Delegate& delegate, + PointerDataDispatcherMaker& dispatcher_maker, DartVM& vm, fml::RefPtr isolate_snapshot, fml::RefPtr shared_snapshot, @@ -51,6 +52,7 @@ Engine::Engine(Delegate& delegate, image_decoder_(task_runners, vm.GetConcurrentWorkerTaskRunner(), io_manager), + task_runners_(std::move(task_runners)), weak_factory_(this) { // Runtime controller is initialized here because it takes a reference to this // object as its delegate. The delegate may be called in the constructor and @@ -60,7 +62,7 @@ Engine::Engine(Delegate& delegate, &vm, // VM std::move(isolate_snapshot), // isolate snapshot std::move(shared_snapshot), // shared snapshot - std::move(task_runners), // task runners + task_runners_, // task runners std::move(io_manager), // io manager image_decoder_.GetWeakPtr(), // image decoder settings_.advisory_script_uri, // advisory script uri @@ -69,6 +71,8 @@ Engine::Engine(Delegate& delegate, settings_.isolate_create_callback, // isolate create callback settings_.isolate_shutdown_callback // isolate shutdown callback ); + + pointer_data_dispatcher_ = dispatcher_maker(*this); } Engine::~Engine() = default; @@ -381,12 +385,12 @@ void Engine::HandleSettingsPlatformMessage(PlatformMessage* message) { } } -void Engine::DispatchPointerDataPacket(const PointerDataPacket& packet, - uint64_t trace_flow_id) { +void Engine::DispatchPointerDataPacket( + std::unique_ptr packet, + uint64_t trace_flow_id) { TRACE_EVENT0("flutter", "Engine::DispatchPointerDataPacket"); TRACE_FLOW_STEP("flutter", "PointerEvent", trace_flow_id); - animator_->EnqueueTraceFlowId(trace_flow_id); - runtime_controller_->DispatchPointerDataPacket(packet); + pointer_data_dispatcher_->DispatchPacket(std::move(packet), trace_flow_id); } void Engine::DispatchSemanticsAction(int id, @@ -434,6 +438,8 @@ void Engine::Render(std::unique_ptr layer_tree) { layer_tree->set_frame_size(frame_size); animator_->Render(std::move(layer_tree)); + + pointer_data_dispatcher_->OnFrameLayerTreeReceived(); } void Engine::UpdateSemantics(SemanticsNodeUpdates update, @@ -462,6 +468,14 @@ FontCollection& Engine::GetFontCollection() { return font_collection_; } +void Engine::DoDispatchPacket(std::unique_ptr packet, + uint64_t trace_flow_id) { + animator_->EnqueueTraceFlowId(trace_flow_id); + if (runtime_controller_) { + runtime_controller_->DispatchPointerDataPacket(*packet); + } +} + void Engine::HandleAssetPlatformMessage(fml::RefPtr message) { fml::RefPtr response = message->response(); if (!response) { diff --git a/shell/common/engine.h b/shell/common/engine.h index e2fa9adf323a3..cc874c237b7f3 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -22,6 +22,8 @@ #include "flutter/runtime/runtime_controller.h" #include "flutter/runtime/runtime_delegate.h" #include "flutter/shell/common/animator.h" +#include "flutter/shell/common/platform_view.h" +#include "flutter/shell/common/pointer_data_dispatcher.h" #include "flutter/shell/common/rasterizer.h" #include "flutter/shell/common/run_configuration.h" #include "flutter/shell/common/shell_io_manager.h" @@ -65,7 +67,7 @@ namespace flutter { /// name and it does happen to be one of the older classes in the /// repository. /// -class Engine final : public RuntimeDelegate { +class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { public: //---------------------------------------------------------------------------- /// @brief Indicates the result of the call to `Engine::Run`. @@ -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 + /// `PlatformView` for engine to create the + /// pointer data dispatcher. Similar to other + /// engine resources, this dispatcher_maker and + /// its returned dispatcher is only safe to be + /// called from the UI thread. /// @param vm An instance of the running Dart VM. /// @param[in] isolate_snapshot The snapshot used to create the root /// isolate. Even though the isolate is not @@ -265,6 +273,7 @@ class Engine final : public RuntimeDelegate { /// GPU. /// Engine(Delegate& delegate, + PointerDataDispatcherMaker& dispatcher_maker, DartVM& vm, fml::RefPtr isolate_snapshot, fml::RefPtr shared_snapshot, @@ -649,7 +658,7 @@ class Engine final : public RuntimeDelegate { /// timeline and allow grouping frames and input /// events into logical chunks. /// - void DispatchPointerDataPacket(const PointerDataPacket& packet, + void DispatchPointerDataPacket(std::unique_ptr packet, uint64_t trace_flow_id); //---------------------------------------------------------------------------- @@ -700,11 +709,23 @@ class Engine final : public RuntimeDelegate { // |RuntimeDelegate| FontCollection& GetFontCollection() override; + // |PointerDataDispatcher::Delegate| + void DoDispatchPacket(std::unique_ptr packet, + uint64_t trace_flow_id) override; + + TaskRunners& task_runners() override { return task_runners_; } + private: Engine::Delegate& delegate_; const Settings settings_; std::unique_ptr animator_; std::unique_ptr runtime_controller_; + + // 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 pointer_data_dispatcher_; + std::string initial_route_; ViewportMetrics viewport_metrics_; std::shared_ptr asset_manager_; @@ -712,6 +733,7 @@ class Engine final : public RuntimeDelegate { bool have_surface_; FontCollection font_collection_; ImageDecoder image_decoder_; + TaskRunners task_runners_; fml::WeakPtrFactory weak_factory_; // |RuntimeDelegate| diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index abfd14b56d41c..9ba950d83b57b 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -11,6 +11,7 @@ void main() {} void nativeReportTimingsCallback(List timings) native 'NativeReportTimingsCallback'; void nativeOnBeginFrame(int microseconds) native 'NativeOnBeginFrame'; +void nativeOnPointerDataPacket() native 'NativeOnPointerDataPacket'; @pragma('vm:entry-point') void reportTimingsMain() { @@ -32,6 +33,16 @@ void onBeginFrameMain() { }; } +@pragma('vm:entry-point') +void onPointerDataPacketMain() { + window.onPointerDataPacket = (PointerDataPacket packet) { + nativeOnPointerDataPacket(); + }; + window.onBeginFrame = (Duration beginTime) { + nativeOnBeginFrame(beginTime.inMicroseconds); + }; +} + @pragma('vm:entry-point') void emptyMain() {} diff --git a/shell/common/input_events_unittests.cc b/shell/common/input_events_unittests.cc new file mode 100644 index 0000000000000..361c237ef3dfa --- /dev/null +++ b/shell/common/input_events_unittests.cc @@ -0,0 +1,250 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/shell/common/shell_test.h" +#include "flutter/testing/testing.h" + +namespace flutter { +namespace testing { + +// Throughout these tests, the choice of time unit is irrelevant as long as all +// times have the same units. +using UnitlessTime = int; + +// Signature of a generator function that takes the frame index as input and +// returns the time of that frame. +using Generator = std::function; + +//---------------------------------------------------------------------------- +/// Simulate n input events where the i-th one is delivered at delivery_time(i). +/// +/// Simulation results will be written into events_consumed_at_frame whose +/// length will be equal to the number of frames drawn. Each element in the +/// vector is the number of input events consumed up to that frame. (We can't +/// return such vector because ASSERT_TRUE requires return type of void.) +/// +/// We assume (and check) that the delivery latency is some base latency plus a +/// random latency where the random latency must be within one frame: +/// +/// 1. latency = delivery_time(i) - j * frame_time = base_latency + +/// random_latency +/// 2. 0 <= base_latency, 0 <= random_latency < frame_time +/// +/// We also assume that there will be at least one input event per frame if +/// there were no latency. Let j = floor( (delivery_time(i) - base_latency) / +/// frame_time ) be the frame index if there were no latency. Then the set of j +/// should be all integers from 0 to continuous_frame_count - 1 for some +/// integer continuous_frame_count. +/// +/// (Note that there coulds be multiple input events within one frame.) +/// +/// The test here is insensitive to the choice of time unit as long as +/// delivery_time and frame_time are in the same unit. +static void TestSimulatedInputEvents( + ShellTest* fixture, + int num_events, + UnitlessTime base_latency, + Generator delivery_time, + UnitlessTime frame_time, + std::vector& events_consumed_at_frame, + bool restart_engine = false) { + ///// Begin constructing shell /////////////////////////////////////////////// + auto settings = fixture->CreateSettingsForFixture(); + std::unique_ptr shell = fixture->CreateShell(settings); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("onPointerDataPacketMain"); + + // The following 4 variables are only accessed in the UI thread by + // nativeOnPointerDataPacket and nativeOnBeginFrame between their + // initializations and `shell.reset()`. + events_consumed_at_frame.clear(); + bool will_draw_new_frame = true; + int events_consumed = 0; + int frame_drawn = 0; + auto nativeOnPointerDataPacket = [&events_consumed_at_frame, + &will_draw_new_frame, &events_consumed, + &frame_drawn](Dart_NativeArguments args) { + events_consumed += 1; + if (will_draw_new_frame) { + frame_drawn += 1; + will_draw_new_frame = false; + events_consumed_at_frame.push_back(events_consumed); + } else { + events_consumed_at_frame.back() = events_consumed; + } + }; + fixture->AddNativeCallback("NativeOnPointerDataPacket", + CREATE_NATIVE_ENTRY(nativeOnPointerDataPacket)); + + auto nativeOnBeginFrame = [&will_draw_new_frame](Dart_NativeArguments args) { + will_draw_new_frame = true; + }; + fixture->AddNativeCallback("NativeOnBeginFrame", + CREATE_NATIVE_ENTRY(nativeOnBeginFrame)); + + ASSERT_TRUE(configuration.IsValid()); + fixture->RunEngine(shell.get(), std::move(configuration)); + + if (restart_engine) { + auto new_configuration = RunConfiguration::InferFromSettings(settings); + new_configuration.SetEntrypoint("onPointerDataPacketMain"); + ASSERT_TRUE(new_configuration.IsValid()); + fixture->RestartEngine(shell.get(), std::move(new_configuration)); + } + ///// End constructing shell ///////////////////////////////////////////////// + + ASSERT_GE(base_latency, 0); + + // Check that delivery_time satisfies our assumptions. + int continuous_frame_count = 0; + for (int i = 0; i < num_events; i += 1) { + // j is the frame index of event i if there were no latency. + int j = static_cast((delivery_time(i) - base_latency) / frame_time); + if (j == continuous_frame_count) { + continuous_frame_count += 1; + } + double random_latency = delivery_time(i) - j * frame_time - base_latency; + ASSERT_GE(random_latency, 0); + ASSERT_LT(random_latency, frame_time); + + // If there were no latency, there should be at least one event per frame. + // Hence j should never skip any integer less than continuous_frame_count. + ASSERT_LT(j, continuous_frame_count); + } + + // i is the input event's index. + // j is the frame's index. + for (int i = 0, j = 0; i < num_events; j += 1) { + double t = j * frame_time; + while (i < num_events && delivery_time(i) <= t) { + ShellTest::DispatchFakePointerData(shell.get()); + i += 1; + } + ShellTest::PumpOneFrame(shell.get()); + } + + shell.reset(); +} + +TEST_F(ShellTest, MissAtMostOneFrameForIrregularInputEvents) { + // We don't use `constexpr int frame_time` here because MSVC doesn't handle + // it well with lambda capture. + UnitlessTime frame_time = 10; + UnitlessTime base_latency = 0.5 * frame_time; + Generator extreme = [frame_time, base_latency](int i) { + return static_cast( + i * frame_time + base_latency + + (i % 2 == 0 ? 0.1 * frame_time : 0.9 * frame_time)); + }; + constexpr int n = 40; + std::vector events_consumed_at_frame; + TestSimulatedInputEvents(this, n, base_latency, extreme, frame_time, + events_consumed_at_frame); + int frame_drawn = events_consumed_at_frame.size(); + ASSERT_GE(frame_drawn, n - 1); + + // Make sure that it also works after an engine restart. + TestSimulatedInputEvents(this, n, base_latency, extreme, frame_time, + events_consumed_at_frame, true /* restart_engine */); + int frame_drawn_after_restart = events_consumed_at_frame.size(); + ASSERT_GE(frame_drawn_after_restart, n - 1); +} + +TEST_F(ShellTest, DelayAtMostOneEventForFasterThanVSyncInputEvents) { + // We don't use `constexpr int frame_time` here because MSVC doesn't handle + // it well with lambda capture. + UnitlessTime frame_time = 10; + UnitlessTime base_latency = 0.2 * frame_time; + Generator double_sampling = [frame_time, base_latency](int i) { + return static_cast(i * 0.5 * frame_time + base_latency); + }; + constexpr int n = 40; + std::vector events_consumed_at_frame; + TestSimulatedInputEvents(this, n, base_latency, double_sampling, frame_time, + events_consumed_at_frame); + + // Draw one extra frame due to delaying a pending packet for the next frame. + int frame_drawn = events_consumed_at_frame.size(); + ASSERT_EQ(frame_drawn, n / 2 + 1); + + for (int i = 0; i < n / 2; i += 1) { + ASSERT_GE(events_consumed_at_frame[i], 2 * i - 1); + } +} + +TEST_F(ShellTest, HandlesActualIphoneXsInputEvents) { + // Actual delivery times measured on iPhone Xs, in the unit of frame_time + // (16.67ms for 60Hz). + constexpr double iphone_xs_times[] = {0.15, + 1.0773046874999999, + 2.1738720703124996, + 3.0579052734374996, + 4.0890087890624995, + 5.0952685546875, + 6.1251708984375, + 7.1253076171875, + 8.125927734374999, + 9.37248046875, + 10.133950195312499, + 11.161201171875, + 12.226992187499999, + 13.1443798828125, + 14.440327148437499, + 15.091684570312498, + 16.138681640625, + 17.126469726562497, + 18.1592431640625, + 19.371372070312496, + 20.033774414062496, + 21.021782226562497, + 22.070053710937497, + 23.325541992187496, + 24.119648437499997, + 25.084262695312496, + 26.077866210937497, + 27.036547851562496, + 28.035073242187497, + 29.081411132812498, + 30.066064453124998, + 31.089360351562497, + 32.086142578125, + 33.4618798828125, + 34.14697265624999, + 35.0513525390625, + 36.136025390624994, + 37.1618408203125, + 38.144472656249995, + 39.201123046875, + 40.4339501953125, + 41.1552099609375, + 42.102128906249995, + 43.0426318359375, + 44.070131835937495, + 45.08862304687499, + 46.091469726562494}; + constexpr int n = sizeof(iphone_xs_times) / sizeof(iphone_xs_times[0]); + // We don't use `constexpr int frame_time` here because MSVC doesn't handle + // it well with lambda capture. + UnitlessTime frame_time = 10000; + for (double base_latency_f = 0; base_latency_f < 1; base_latency_f += 0.1) { + // Everything is converted to int to avoid floating point error in + // TestSimulatedInputEvents. + UnitlessTime base_latency = + static_cast(base_latency_f * frame_time); + Generator iphone_xs_generator = [frame_time, iphone_xs_times, + base_latency](int i) { + return base_latency + + static_cast(iphone_xs_times[i] * frame_time); + }; + std::vector events_consumed_at_frame; + TestSimulatedInputEvents(this, n, base_latency, iphone_xs_generator, + frame_time, events_consumed_at_frame); + int frame_drawn = events_consumed_at_frame.size(); + ASSERT_GE(frame_drawn, n - 1); + } +} + +} // namespace testing +} // namespace flutter diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 4a84f27f4e456..058ad55d0c801 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -88,6 +88,12 @@ sk_sp PlatformView::CreateResourceContext() const { void PlatformView::ReleaseResourceContext() const {} +PointerDataDispatcherMaker PlatformView::GetDispatcherMaker() { + return [](DefaultPointerDataDispatcher::Delegate& delegate) { + return std::make_unique(delegate); + }; +} + fml::WeakPtr PlatformView::GetWeakPtr() const { return weak_factory_.GetWeakPtr(); } diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 414399150511b..9bbc0a6baf935 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -16,6 +16,7 @@ #include "flutter/lib/ui/window/platform_message.h" #include "flutter/lib/ui/window/pointer_data_packet.h" #include "flutter/lib/ui/window/viewport_metrics.h" +#include "flutter/shell/common/pointer_data_dispatcher.h" #include "flutter/shell/common/surface.h" #include "flutter/shell/common/vsync_waiter.h" #include "third_party/skia/include/core/SkSize.h" @@ -418,17 +419,23 @@ class PlatformView { /// virtual void ReleaseResourceContext() const; + //-------------------------------------------------------------------------- + /// @brief Returns a platform-specific PointerDataDispatcherMaker so the + /// `Engien` can construct the PointerDataPacketDispatcher based + /// on platforms. + virtual PointerDataDispatcherMaker GetDispatcherMaker(); + //---------------------------------------------------------------------------- /// @brief Returns a weak pointer to the platform view. Since the - /// platform view may only be created, accessed and destroyed on - /// the platform thread, any access to the platform view from a - /// non-platform task runner needs a weak pointer to the platform - /// view along with a reference to the platform task runner. A - /// task must be posted to the platform task runner with the weak - /// pointer captured in the same. The platform view method may - /// only be called in the posted task once the weak pointer - /// validity has been checked. This method is used by callers to - /// obtain that weak pointer. + /// platform view may only be created, accessed and destroyed + /// on the platform thread, any access to the platform view + /// from a non-platform task runner needs a weak pointer to + /// the platform view along with a reference to the platform + /// task runner. A task must be posted to the platform task + /// runner with the weak pointer captured in the same. The + /// platform view method may only be called in the posted task + /// once the weak pointer validity has been checked. This + /// method is used by callers to obtain that weak pointer. /// /// @return The weak pointer to the platform view. /// @@ -446,13 +453,14 @@ class PlatformView { //---------------------------------------------------------------------------- /// @brief Sets a callback that gets executed when the rasterizer renders - /// the next frame. Due to the asynchronous nature of rendering in - /// Flutter, embedders usually add a placeholder over the - /// contents in which Flutter is going to render when Flutter is - /// first initialized. This callback may be used as a signal to - /// remove that placeholder. The callback is executed on the - /// render task runner and not the platform task runner. It is - /// the embedder's responsibility to re-thread as necessary. + /// the next frame. Due to the asynchronous nature of + /// rendering in Flutter, embedders usually add a placeholder + /// over the contents in which Flutter is going to render when + /// Flutter is first initialized. This callback may be used as + /// a signal to remove that placeholder. The callback is + /// executed on the render task runner and not the platform + /// task runner. It is the embedder's responsibility to + /// re-thread as necessary. /// /// @attention The callback is executed on the render task runner and not the /// platform task runner. Embedders must re-thread as necessary. @@ -465,8 +473,8 @@ class PlatformView { //---------------------------------------------------------------------------- /// @brief Dispatches pointer events from the embedder to the /// framework. Each pointer data packet may contain multiple - /// pointer input events. Each call to this method wakes up the UI - /// thread. + /// pointer input events. Each call to this method wakes up + /// the UI thread. /// /// @param[in] packet The pointer data packet to dispatch to the framework. /// @@ -475,16 +483,17 @@ class PlatformView { //-------------------------------------------------------------------------- /// @brief Used by the embedder to specify a texture that it wants the /// rasterizer to composite within the Flutter layer tree. All - /// textures must have a unique identifier. When the rasterizer - /// encounters an external texture within its hierarchy, it gives - /// the embedder a chance to update that texture on the GPU thread - /// before it composites the same on-screen. + /// textures must have a unique identifier. When the + /// rasterizer encounters an external texture within its + /// hierarchy, it gives the embedder a chance to update that + /// texture on the GPU thread before it composites the same + /// on-screen. /// /// @attention This method must only be called once per texture. When the - /// texture is updated, calling `MarkTextureFrameAvailable` with - /// the specified texture identifier is sufficient to make Flutter - /// re-render the frame with the updated texture composited - /// in-line. + /// texture is updated, calling `MarkTextureFrameAvailable` + /// with the specified texture identifier is sufficient to + /// make Flutter re-render the frame with the updated texture + /// composited in-line. /// /// @see UnregisterTexture, MarkTextureFrameAvailable /// @@ -494,10 +503,11 @@ class PlatformView { void RegisterTexture(std::shared_ptr texture); //-------------------------------------------------------------------------- - /// @brief Used by the embedder to notify the rasterizer that it will no - /// longer attempt to composite the specified texture within the - /// layer tree. This allows the rasterizer to collect associated - /// resources. + /// @brief Used by the embedder to notify the rasterizer that it will + /// no + /// longer attempt to composite the specified texture within + /// the layer tree. This allows the rasterizer to collect + /// associated resources. /// /// @attention This call must only be called once per texture identifier. /// @@ -514,13 +524,14 @@ class PlatformView { /// of the previously registered texture have been updated. /// Typically, Flutter will only render a frame if there is an /// updated layer tree. However, in cases where the layer tree - /// is static but one of the externally composited textures has - /// been updated by the embedder, the embedder needs to notify - /// the rasterizer to render a new frame. In such cases, the - /// existing layer tree may be reused with the frame re-composited - /// with all updated external textures. Unlike the calls to - /// register and unregister the texture, this call must be made - /// each time a new texture frame is available. + /// is static but one of the externally composited textures + /// has been updated by the embedder, the embedder needs to + /// notify the rasterizer to render a new frame. In such + /// cases, the existing layer tree may be reused with the + /// frame re-composited with all updated external textures. + /// Unlike the calls to register and unregister the texture, + /// this call must be made each time a new texture frame is + /// available. /// /// @see RegisterTexture, UnregisterTexture /// @@ -536,8 +547,8 @@ class PlatformView { SkISize size_; fml::WeakPtrFactory weak_factory_; - // Unlike all other methods on the platform view, this is called on the GPU - // task runner. + // Unlike all other methods on the platform view, this is called on the + // GPU task runner. virtual std::unique_ptr CreateRenderingSurface(); private: diff --git a/shell/common/pointer_data_dispatcher.cc b/shell/common/pointer_data_dispatcher.cc new file mode 100644 index 0000000000000..0e1101b94cf4a --- /dev/null +++ b/shell/common/pointer_data_dispatcher.cc @@ -0,0 +1,74 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/shell/common/pointer_data_dispatcher.h" + +namespace flutter { + +PointerDataDispatcher::~PointerDataDispatcher() = default; +DefaultPointerDataDispatcher::~DefaultPointerDataDispatcher() = default; +SmoothPointerDataDispatcher::~SmoothPointerDataDispatcher() = default; + +void DefaultPointerDataDispatcher::DispatchPacket( + std::unique_ptr packet, + uint64_t trace_flow_id) { + delegate_.DoDispatchPacket(std::move(packet), trace_flow_id); +} + +// Intentional no-op. +void DefaultPointerDataDispatcher::OnFrameLayerTreeReceived() {} + +void SmoothPointerDataDispatcher::DispatchPacket( + std::unique_ptr packet, + uint64_t trace_flow_id) { + if (is_pointer_data_in_progress_) { + if (pending_packet_ != nullptr) { + DispatchPendingPacket(); + } + pending_packet_ = std::move(packet); + pending_trace_flow_id_ = trace_flow_id; + } else { + FML_DCHECK(pending_packet_ == nullptr); + DefaultPointerDataDispatcher::DispatchPacket(std::move(packet), + trace_flow_id); + } + is_pointer_data_in_progress_ = true; +} + +void SmoothPointerDataDispatcher::OnFrameLayerTreeReceived() { + if (is_pointer_data_in_progress_) { + if (pending_packet_ != nullptr) { + // This is already in the UI thread. However, method + // `OnFrameLayerTreeReceived` is called by `Engine::Render` (a part of the + // `VSYNC` UI thread task) which is in Flutter framework's + // `SchedulerPhase.persistentCallbacks` phase. In that phase, no pointer + // data packet is allowed to be fired because the framework requires such + // phase to be executed synchronously without being interrupted. Hence + // we'll post a new UI thread task to fire the packet after `VSYNC` task + // is done. When a non-VSYNC UI thread task (like the following one) is + // run, the Flutter framework is always in `SchedulerPhase.idle` phase). + delegate_.task_runners().GetUITaskRunner()->PostTask( + // Use and validate a `fml::WeakPtr` because this dispatcher might + // have been destructed with engine when the task is run. + [dispatcher = weak_factory_.GetWeakPtr()]() { + if (dispatcher) { + dispatcher->DispatchPendingPacket(); + } + }); + } else { + is_pointer_data_in_progress_ = false; + } + } +} + +void SmoothPointerDataDispatcher::DispatchPendingPacket() { + FML_DCHECK(pending_packet_ != nullptr); + FML_DCHECK(is_pointer_data_in_progress_); + DefaultPointerDataDispatcher::DispatchPacket(std::move(pending_packet_), + pending_trace_flow_id_); + pending_packet_ = nullptr; + pending_trace_flow_id_ = -1; +} + +} // namespace flutter diff --git a/shell/common/pointer_data_dispatcher.h b/shell/common/pointer_data_dispatcher.h new file mode 100644 index 0000000000000..86f865d7f5460 --- /dev/null +++ b/shell/common/pointer_data_dispatcher.h @@ -0,0 +1,174 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef POINTER_DATA_DISPATCHER_H_ +#define POINTER_DATA_DISPATCHER_H_ + +#include "flutter/runtime/runtime_controller.h" +#include "flutter/shell/common/animator.h" + +namespace flutter { + +class PointerDataDispatcher; + +//------------------------------------------------------------------------------ +/// The `Engine` pointer data dispatcher that forwards the packet received from +/// `PlatformView::DispatchPointerDataPacket` on the platform thread, to +/// `Window::DispatchPointerDataPacket` on the UI thread. +/// +/// This class is used to filter the packets so the Flutter framework on the UI +/// thread will receive packets with some desired properties. See +/// `SmoothPointerDataDispatcher` for an example which filters irregularly +/// delivered packets, and dispatches them in sync with the VSYNC signal. +/// +/// This object will be owned by the engine because it relies on the engine's +/// `Animator` (which owns `VsyncWaiter`) and `RuntomeController` to do the +/// filtering. This object is currently designed to be only called from the UI +/// thread (no thread safety is guaranteed). +/// +/// The `PlatformView` decides which subclass of `PointerDataDispatcher` is +/// constructed by sending a `PointerDataDispatcherMaker` to the engine's +/// constructor in `Shell::CreateShellOnPlatformThread`. This is needed because: +/// (1) Different platforms (e.g., Android, iOS) have different dispatchers +/// so the decision has to be made per `PlatformView`. +/// (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 +/// construction of the `PointerDataDispatcher`. +class PointerDataDispatcher { + public: + /// The interface for Engine to implement. + class Delegate { + public: + /// Actually dispatch the packet using Engine's `animator_` and + /// `runtime_controller_`. + virtual void DoDispatchPacket(std::unique_ptr packet, + uint64_t trace_flow_id) = 0; + + /// Get the task runners to schedule tasks on specific threads. + virtual TaskRunners& task_runners() = 0; + }; + + //---------------------------------------------------------------------------- + /// @brief Signal that `PlatformView` has a packet to be dispatched. + /// + /// @param[in] packet The `PointerDataPacket` to be dispatched. + /// @param[in] trace_flow_id The id for `Animator::EnqueueTraceFlowId`. + virtual void DispatchPacket(std::unique_ptr packet, + uint64_t trace_flow_id) = 0; + //---------------------------------------------------------------------------- + /// @brief Signal that the engine has received a frame layer tree for + /// rendering. This signal is supposed to be regularly delivered + /// at the same frequency of VSYNC. Such frame layer tree is + /// usually a result of a previous packet sent to the Flutter + /// framework. + virtual void OnFrameLayerTreeReceived() = 0; + + //---------------------------------------------------------------------------- + /// @brief Default destructor. + virtual ~PointerDataDispatcher(); +}; + +//------------------------------------------------------------------------------ +/// The default dispatcher that forwards the packet without any modification. +/// +class DefaultPointerDataDispatcher : public PointerDataDispatcher { + public: + DefaultPointerDataDispatcher(Delegate& delegate) : delegate_(delegate) {} + + // |PointerDataDispatcer| + void DispatchPacket(std::unique_ptr packet, + uint64_t trace_flow_id) override; + + // |PointerDataDispatcer| + void OnFrameLayerTreeReceived() override; + + virtual ~DefaultPointerDataDispatcher(); + + protected: + Delegate& delegate_; + + FML_DISALLOW_COPY_AND_ASSIGN(DefaultPointerDataDispatcher); +}; + +//------------------------------------------------------------------------------ +/// 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. +/// +/// It works as follows: +/// +/// When `DispatchPacket` is called while a preivous pointer data dispatch is +/// still in progress (its frame isn't finished yet), it means that an input +/// event is delivered to us too fast. That potentially means a later event will +/// be too late which could cause the missing of a frame. Hence we'll cache it +/// in `pending_packet_` for the next frame to smooth it out. +/// +/// If the input event is sent to us regularly at the same rate of VSYNC (say +/// at 60Hz), this would be identical to `DefaultPointerDataDispatcher` where +/// `runtime_controller_->DispatchPointerDataPacket` is always called right +/// away. That's because `is_pointer_data_in_progress_` will always be false +/// when `DispatchPacket` is called since it will be cleared by the end of a +/// frame through `OnFrameLayerTreeReceived`. This is the case for all +/// Android/iOS devices before iPhone X/XS. +/// +/// If the input event is irregular, but with a random latency of no more than +/// one frame, this would guarantee that we'll miss at most 1 frame. Without +/// this, we could miss half of the frames. +/// +/// If the input event is delivered at a higher rate than that of VSYNC, this +/// would at most add a latency of one event delivery. For example, if the +/// input event is delivered at 120Hz (this is only true for iPad pro, not even +/// iPhone X), this may delay the handling of an input event by 8ms. +/// +/// The assumption of this solution is that the sampling itself is still +/// regular. Only the event delivery is allowed to be irregular. So far this +/// assumption seems to hold on all devices. If it's changed in the future, +/// we'll need a different solution. +/// +/// See also input_events_unittests.cc where we test all our claims above. +class SmoothPointerDataDispatcher : public DefaultPointerDataDispatcher { + public: + SmoothPointerDataDispatcher(Delegate& delegate) + : DefaultPointerDataDispatcher(delegate), weak_factory_(this) {} + + // |PointerDataDispatcer| + void DispatchPacket(std::unique_ptr packet, + uint64_t trace_flow_id) override; + + // |PointerDataDispatcer| + void OnFrameLayerTreeReceived() override; + + virtual ~SmoothPointerDataDispatcher(); + + private: + // If non-null, this will be a pending pointer data packet for the next frame + // to consume. This is used to smooth out the irregular drag events delivery. + // See also `DispatchPointerDataPacket` and input_events_unittests.cc. + std::unique_ptr pending_packet_; + int pending_trace_flow_id_ = -1; + + bool is_pointer_data_in_progress_ = false; + + fml::WeakPtrFactory weak_factory_; + + void DispatchPendingPacket(); + + FML_DISALLOW_COPY_AND_ASSIGN(SmoothPointerDataDispatcher); +}; + +//-------------------------------------------------------------------------- +/// @brief Signature for constructing PointerDataDispatcher. +/// +/// @param[in] delegate the `Flutter::Engine` +/// +using PointerDataDispatcherMaker = + std::function( + PointerDataDispatcher::Delegate&)>; + +} // namespace flutter + +#endif // POINTER_DATA_DISPATCHER_H_ diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 93c6c1042286d..804dcb2b722a7 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -98,6 +98,10 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( io_manager_promise.set_value(std::move(io_manager)); }); + // Send dispatcher_maker to the engine constructor because shell won't have + // platform_view set until Shell::Setup is called later. + auto dispatcher_maker = platform_view->GetDispatcherMaker(); + // Create the engine on the UI thread. std::promise> engine_promise; auto engine_future = engine_promise.get_future(); @@ -105,6 +109,7 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( shell->GetTaskRunners().GetUITaskRunner(), fml::MakeCopyable([&engine_promise, // shell = shell.get(), // + &dispatcher_maker, // isolate_snapshot = std::move(isolate_snapshot), // shared_snapshot = std::move(shared_snapshot), // vsync_waiter = std::move(vsync_waiter), // @@ -120,6 +125,7 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( engine_promise.set_value(std::make_unique( *shell, // + dispatcher_maker, // *shell->GetDartVM(), // std::move(isolate_snapshot), // std::move(shared_snapshot), // @@ -715,11 +721,11 @@ void Shell::OnPlatformViewDispatchPointerDataPacket( TRACE_FLOW_BEGIN("flutter", "PointerEvent", next_pointer_flow_id_); FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - task_runners_.GetUITaskRunner()->PostTask(fml::MakeCopyable( - [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), + flow_id = next_pointer_flow_id_]() mutable { if (engine) { - engine->DispatchPointerDataPacket(*packet, flow_id); + engine->DispatchPointerDataPacket(std::move(packet), flow_id); } })); next_pointer_flow_id_++; diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index e541d908c5586..3ede4058be9a6 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -101,6 +101,18 @@ void ShellTest::RunEngine(Shell* shell, RunConfiguration configuration) { latch.Wait(); } +void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { + fml::AutoResetWaitableEvent latch; + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetUITaskRunner(), + [shell, &latch, &configuration]() { + bool restarted = shell->engine_->Restart(std::move(configuration)); + ASSERT_TRUE(restarted); + latch.Signal(); + }); + latch.Wait(); +} + void ShellTest::PumpOneFrame(Shell* shell) { // Set viewport to nonempty, and call Animator::BeginFrame to make the layer // tree pipeline nonempty. Without either of this, the layer tree below @@ -132,6 +144,16 @@ void ShellTest::PumpOneFrame(Shell* shell) { latch.Wait(); } +void ShellTest::DispatchFakePointerData(Shell* shell) { + fml::AutoResetWaitableEvent latch; + shell->GetTaskRunners().GetPlatformTaskRunner()->PostTask([&latch, shell]() { + auto packet = std::make_unique(1); + shell->OnPlatformViewDispatchPointerDataPacket(std::move(packet)); + latch.Signal(); + }); + latch.Wait(); +} + int ShellTest::UnreportedTimingsCount(Shell* shell) { return shell->unreported_timings_.size(); } @@ -222,6 +244,13 @@ std::unique_ptr ShellTestPlatformView::CreateRenderingSurface() { return std::make_unique(this, true); } +// |PlatformView| +PointerDataDispatcherMaker ShellTestPlatformView::GetDispatcherMaker() { + return [](DefaultPointerDataDispatcher::Delegate& delegate) { + return std::make_unique(delegate); + }; +} + // |GPUSurfaceGLDelegate| bool ShellTestPlatformView::GLContextMakeCurrent() { return gl_surface_.MakeCurrent(); diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index 55436a82a29ed..ae6c92560fa09 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -41,8 +41,10 @@ class ShellTest : public ThreadTest { static void PlatformViewNotifyCreated( Shell* shell); // This creates the surface static void RunEngine(Shell* shell, RunConfiguration configuration); + static void RestartEngine(Shell* shell, RunConfiguration configuration); static void PumpOneFrame(Shell* shell); + static void DispatchFakePointerData(Shell* shell); // Declare |UnreportedTimingsCount|, |GetNeedsReportTimings| and // |SetNeedsReportTimings| inside |ShellTest| mainly for easier friend class @@ -84,6 +86,9 @@ class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { // |PlatformView| std::unique_ptr CreateRenderingSurface() override; + // |PlatformView| + PointerDataDispatcherMaker GetDispatcherMaker() override; + // |GPUSurfaceGLDelegate| bool GLContextMakeCurrent() override; diff --git a/shell/platform/darwin/ios/platform_view_ios.h b/shell/platform/darwin/ios/platform_view_ios.h index eeaca3dd9df50..51d00e86370b1 100644 --- a/shell/platform/darwin/ios/platform_view_ios.h +++ b/shell/platform/darwin/ios/platform_view_ios.h @@ -37,6 +37,9 @@ class PlatformViewIOS final : public PlatformView { void RegisterExternalTexture(int64_t id, NSObject* texture); + // |PlatformView| + PointerDataDispatcherMaker GetDispatcherMaker() override; + fml::scoped_nsprotocol GetTextInputPlugin() const; void SetTextInputPlugin(fml::scoped_nsprotocol plugin); diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 778823493977a..ce3f25d6ca8db 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -98,6 +98,12 @@ new AccessibilityBridge(static_cast(owner_controller_.get().view), } } +PointerDataDispatcherMaker PlatformViewIOS::GetDispatcherMaker() { + return [](DefaultPointerDataDispatcher::Delegate& delegate) { + return std::make_unique(delegate); + }; +} + void PlatformViewIOS::RegisterExternalTexture(int64_t texture_id, NSObject* texture) { RegisterTexture(std::make_shared(texture_id, texture)); From fda31bfd4ae40679066dcc3804e3a9eeba017484 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 20 Sep 2019 22:56:13 -0700 Subject: [PATCH 2/6] Fix by adding a secondary VSYNC callback --- shell/common/animator.cc | 4 ++ shell/common/animator.h | 2 + shell/common/engine.cc | 6 +- shell/common/engine.h | 2 +- shell/common/pointer_data_dispatcher.cc | 40 ++++-------- shell/common/pointer_data_dispatcher.h | 30 +++++---- shell/common/vsync_waiter.cc | 81 ++++++++++++++++++------- shell/common/vsync_waiter.h | 5 ++ 8 files changed, 102 insertions(+), 68 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index c987805784c2f..7e3f63eff91aa 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -244,4 +244,8 @@ void Animator::AwaitVSync() { delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_); } +void Animator::ScheduleSecondaryVsyncCallback(std::function callback) { + waiter_->ScheduleSecondaryCallback(std::move(callback)); +} + } // namespace flutter diff --git a/shell/common/animator.h b/shell/common/animator.h index 9f84468c01b78..7efe79ac5d80c 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -52,6 +52,8 @@ class Animator final { void Render(std::unique_ptr layer_tree); + void ScheduleSecondaryVsyncCallback(std::function callback); + void Start(); void Stop(); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 18dd07cc82751..517479edcd1af 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -438,8 +438,6 @@ void Engine::Render(std::unique_ptr layer_tree) { layer_tree->set_frame_size(frame_size); animator_->Render(std::move(layer_tree)); - - pointer_data_dispatcher_->OnFrameLayerTreeReceived(); } void Engine::UpdateSemantics(SemanticsNodeUpdates update, @@ -476,6 +474,10 @@ void Engine::DoDispatchPacket(std::unique_ptr packet, } } +void Engine::ScheduleSecondaryVsyncCallback(std::function callback) { + animator_->ScheduleSecondaryVsyncCallback(std::move(callback)); +} + void Engine::HandleAssetPlatformMessage(fml::RefPtr message) { fml::RefPtr response = message->response(); if (!response) { diff --git a/shell/common/engine.h b/shell/common/engine.h index cc874c237b7f3..a913d27e3e16d 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -713,7 +713,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { void DoDispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) override; - TaskRunners& task_runners() override { return task_runners_; } + void ScheduleSecondaryVsyncCallback(std::function callback) override; private: Engine::Delegate& delegate_; diff --git a/shell/common/pointer_data_dispatcher.cc b/shell/common/pointer_data_dispatcher.cc index 0e1101b94cf4a..508fc0c09c113 100644 --- a/shell/common/pointer_data_dispatcher.cc +++ b/shell/common/pointer_data_dispatcher.cc @@ -16,9 +16,6 @@ void DefaultPointerDataDispatcher::DispatchPacket( delegate_.DoDispatchPacket(std::move(packet), trace_flow_id); } -// Intentional no-op. -void DefaultPointerDataDispatcher::OnFrameLayerTreeReceived() {} - void SmoothPointerDataDispatcher::DispatchPacket( std::unique_ptr packet, uint64_t trace_flow_id) { @@ -34,32 +31,20 @@ void SmoothPointerDataDispatcher::DispatchPacket( trace_flow_id); } is_pointer_data_in_progress_ = true; + ScheduleSecondaryVsyncCallback(); } -void SmoothPointerDataDispatcher::OnFrameLayerTreeReceived() { - if (is_pointer_data_in_progress_) { - if (pending_packet_ != nullptr) { - // This is already in the UI thread. However, method - // `OnFrameLayerTreeReceived` is called by `Engine::Render` (a part of the - // `VSYNC` UI thread task) which is in Flutter framework's - // `SchedulerPhase.persistentCallbacks` phase. In that phase, no pointer - // data packet is allowed to be fired because the framework requires such - // phase to be executed synchronously without being interrupted. Hence - // we'll post a new UI thread task to fire the packet after `VSYNC` task - // is done. When a non-VSYNC UI thread task (like the following one) is - // run, the Flutter framework is always in `SchedulerPhase.idle` phase). - delegate_.task_runners().GetUITaskRunner()->PostTask( - // Use and validate a `fml::WeakPtr` because this dispatcher might - // have been destructed with engine when the task is run. - [dispatcher = weak_factory_.GetWeakPtr()]() { - if (dispatcher) { - dispatcher->DispatchPendingPacket(); - } - }); - } else { - is_pointer_data_in_progress_ = false; - } - } +void SmoothPointerDataDispatcher::ScheduleSecondaryVsyncCallback() { + delegate_.ScheduleSecondaryVsyncCallback( + [dispatcher = weak_factory_.GetWeakPtr()]() { + if (dispatcher && dispatcher->is_pointer_data_in_progress_) { + if (dispatcher->pending_packet_ != nullptr) { + dispatcher->DispatchPendingPacket(); + } else { + dispatcher->is_pointer_data_in_progress_ = false; + } + } + }); } void SmoothPointerDataDispatcher::DispatchPendingPacket() { @@ -69,6 +54,7 @@ void SmoothPointerDataDispatcher::DispatchPendingPacket() { pending_trace_flow_id_); pending_packet_ = nullptr; pending_trace_flow_id_ = -1; + ScheduleSecondaryVsyncCallback(); } } // namespace flutter diff --git a/shell/common/pointer_data_dispatcher.h b/shell/common/pointer_data_dispatcher.h index 86f865d7f5460..6fb2e9a422672 100644 --- a/shell/common/pointer_data_dispatcher.h +++ b/shell/common/pointer_data_dispatcher.h @@ -47,8 +47,17 @@ class PointerDataDispatcher { virtual void DoDispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) = 0; - /// Get the task runners to schedule tasks on specific threads. - virtual TaskRunners& task_runners() = 0; + //---------------------------------------------------------------------------- + /// @brief Schedule a secondary callback to be executed right after the + /// main `Animator::RequestFrame` callback. + /// + /// If there is no RequestFrame callback, this secondary callback + /// will still be executed at vsync. + /// + /// This callback is used to provide the vsync signal needed by + /// `SmoothPointerDataDispatcher`. + virtual void ScheduleSecondaryVsyncCallback( + std::function callback) = 0; }; //---------------------------------------------------------------------------- @@ -58,13 +67,6 @@ class PointerDataDispatcher { /// @param[in] trace_flow_id The id for `Animator::EnqueueTraceFlowId`. virtual void DispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) = 0; - //---------------------------------------------------------------------------- - /// @brief Signal that the engine has received a frame layer tree for - /// rendering. This signal is supposed to be regularly delivered - /// at the same frequency of VSYNC. Such frame layer tree is - /// usually a result of a previous packet sent to the Flutter - /// framework. - virtual void OnFrameLayerTreeReceived() = 0; //---------------------------------------------------------------------------- /// @brief Default destructor. @@ -82,9 +84,6 @@ class DefaultPointerDataDispatcher : public PointerDataDispatcher { void DispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) override; - // |PointerDataDispatcer| - void OnFrameLayerTreeReceived() override; - virtual ~DefaultPointerDataDispatcher(); protected: @@ -112,7 +111,7 @@ class DefaultPointerDataDispatcher : public PointerDataDispatcher { /// `runtime_controller_->DispatchPointerDataPacket` is always called right /// away. That's because `is_pointer_data_in_progress_` will always be false /// when `DispatchPacket` is called since it will be cleared by the end of a -/// frame through `OnFrameLayerTreeReceived`. This is the case for all +/// frame through `ScheduleSecondaryVsyncCallback`. This is the case for all /// Android/iOS devices before iPhone X/XS. /// /// If the input event is irregular, but with a random latency of no more than @@ -139,9 +138,6 @@ class SmoothPointerDataDispatcher : public DefaultPointerDataDispatcher { void DispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) override; - // |PointerDataDispatcer| - void OnFrameLayerTreeReceived() override; - virtual ~SmoothPointerDataDispatcher(); private: @@ -157,6 +153,8 @@ class SmoothPointerDataDispatcher : public DefaultPointerDataDispatcher { void DispatchPendingPacket(); + void ScheduleSecondaryVsyncCallback(); + FML_DISALLOW_COPY_AND_ASSIGN(SmoothPointerDataDispatcher); }; diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index aa3637221df0a..b2efafa8c0079 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -46,6 +46,34 @@ void VsyncWaiter::AsyncWaitForVsync(Callback callback) { return; } callback_ = std::move(callback); + if (secondary_callback_) { + // Return directly as `AwaitVSync` is already called by + // `ScheduleSecondaryCallback`. + return; + } + } + AwaitVSync(); +} + +void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { + if (!callback) { + return; + } + + TRACE_EVENT0("flutter", "ScheduleSecondaryCallback"); + + { + std::scoped_lock lock(callback_mutex_); + if (secondary_callback_) { + // Multiple schedules must result in a single callback per frame interval. + return; + } + secondary_callback_ = std::move(callback); + if (callback_) { + // Return directly as `AwaitVSync` is already called by + // `AsyncWaitForVsync`. + return; + } } AwaitVSync(); } @@ -53,13 +81,15 @@ void VsyncWaiter::AsyncWaitForVsync(Callback callback) { void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, fml::TimePoint frame_target_time) { Callback callback; + std::function secondary_callback; { std::scoped_lock lock(callback_mutex_); callback = std::move(callback_); + secondary_callback = std::move(secondary_callback_); } - if (!callback) { + if (!callback && !secondary_callback) { // This means that the vsync waiter implementation fired a callback for a // request we did not make. This is a paranoid check but we still want to // make sure we catch misbehaving vsync implementations. @@ -67,27 +97,34 @@ void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, return; } - auto flow_identifier = fml::tracing::TraceNonce(); - - // The base trace ensures that flows have a root to begin from if one does not - // exist. The trace viewer will ignore traces that have no base event trace. - // While all our message loops insert a base trace trace - // (MessageLoop::RunExpiredTasks), embedders may not. - TRACE_EVENT0("flutter", "VsyncFireCallback"); - - TRACE_FLOW_BEGIN("flutter", kVsyncFlowName, flow_identifier); - - task_runners_.GetUITaskRunner()->PostTaskForTime( - [callback, flow_identifier, frame_start_time, frame_target_time]() { - FML_TRACE_EVENT("flutter", kVsyncTraceName, "StartTime", - frame_start_time, "TargetTime", frame_target_time); - fml::tracing::TraceEventAsyncComplete( - "flutter", "VsyncSchedulingOverhead", fml::TimePoint::Now(), - frame_start_time); - callback(frame_start_time, frame_target_time); - TRACE_FLOW_END("flutter", kVsyncFlowName, flow_identifier); - }, - frame_start_time); + if (callback) { + auto flow_identifier = fml::tracing::TraceNonce(); + + // The base trace ensures that flows have a root to begin from if one does + // not exist. The trace viewer will ignore traces that have no base event + // trace. While all our message loops insert a base trace trace + // (MessageLoop::RunExpiredTasks), embedders may not. + TRACE_EVENT0("flutter", "VsyncFireCallback"); + + TRACE_FLOW_BEGIN("flutter", kVsyncFlowName, flow_identifier); + + task_runners_.GetUITaskRunner()->PostTaskForTime( + [callback, flow_identifier, frame_start_time, frame_target_time]() { + FML_TRACE_EVENT("flutter", kVsyncTraceName, "StartTime", + frame_start_time, "TargetTime", frame_target_time); + fml::tracing::TraceEventAsyncComplete( + "flutter", "VsyncSchedulingOverhead", fml::TimePoint::Now(), + frame_start_time); + callback(frame_start_time, frame_target_time); + TRACE_FLOW_END("flutter", kVsyncFlowName, flow_identifier); + }, + frame_start_time); + } + + if (secondary_callback) { + task_runners_.GetUITaskRunner()->PostTaskForTime( + std::move(secondary_callback), frame_start_time); + } } float VsyncWaiter::GetDisplayRefreshRate() const { diff --git a/shell/common/vsync_waiter.h b/shell/common/vsync_waiter.h index 5e80dd4c09c6b..6b7bcd35eaf0a 100644 --- a/shell/common/vsync_waiter.h +++ b/shell/common/vsync_waiter.h @@ -26,6 +26,8 @@ class VsyncWaiter : public std::enable_shared_from_this { void AsyncWaitForVsync(Callback callback); + void ScheduleSecondaryCallback(std::function callback); + static constexpr float kUnknownRefreshRateFPS = 0.0; // Get the display's maximum refresh rate in the unit of frame per second. @@ -55,6 +57,9 @@ class VsyncWaiter : public std::enable_shared_from_this { std::mutex callback_mutex_; Callback callback_ FML_GUARDED_BY(callback_mutex_); + std::mutex secondary_callback_mutex_; + std::function secondary_callback_ FML_GUARDED_BY(callback_mutex_); + FML_DISALLOW_COPY_AND_ASSIGN(VsyncWaiter); }; From a6f62c7614a17d9d7641d2eea00fc0be6c0556af Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Sat, 21 Sep 2019 21:35:16 -0700 Subject: [PATCH 3/6] Update unit tests --- shell/common/input_events_unittests.cc | 36 ++++++++------ shell/common/shell_test.cc | 67 ++++++++++++++++++++++++++ shell/common/shell_test.h | 38 +++++++++++++++ shell/common/vsync_waiter.cc | 2 + 4 files changed, 128 insertions(+), 15 deletions(-) diff --git a/shell/common/input_events_unittests.cc b/shell/common/input_events_unittests.cc index 361c237ef3dfa..79d3baa1d5b4a 100644 --- a/shell/common/input_events_unittests.cc +++ b/shell/common/input_events_unittests.cc @@ -78,12 +78,6 @@ static void TestSimulatedInputEvents( fixture->AddNativeCallback("NativeOnPointerDataPacket", CREATE_NATIVE_ENTRY(nativeOnPointerDataPacket)); - auto nativeOnBeginFrame = [&will_draw_new_frame](Dart_NativeArguments args) { - will_draw_new_frame = true; - }; - fixture->AddNativeCallback("NativeOnBeginFrame", - CREATE_NATIVE_ENTRY(nativeOnBeginFrame)); - ASSERT_TRUE(configuration.IsValid()); fixture->RunEngine(shell.get(), std::move(configuration)); @@ -114,18 +108,30 @@ static void TestSimulatedInputEvents( ASSERT_LT(j, continuous_frame_count); } - // i is the input event's index. - // j is the frame's index. - for (int i = 0, j = 0; i < num_events; j += 1) { - double t = j * frame_time; - while (i < num_events && delivery_time(i) <= t) { - ShellTest::DispatchFakePointerData(shell.get()); - i += 1; + // This has to be running on a different thread than Platform thread to avoid + // dead locks. + auto simulation = std::async(std::launch::async, [&]() { + // i is the input event's index. + // j is the frame's index. + for (int i = 0, j = 0; i < num_events; j += 1) { + double t = j * frame_time; + while (i < num_events && delivery_time(i) <= t) { + ShellTest::DispatchFakePointerData(shell.get()); + i += 1; + } + ShellTest::VSyncFlush(shell.get(), will_draw_new_frame); } - ShellTest::PumpOneFrame(shell.get()); - } + // Finally, issue a vsync for the pending event that may be generated duing + // the last vsync. + ShellTest::VSyncFlush(shell.get(), will_draw_new_frame); + }); + simulation.wait(); shell.reset(); + + // Make sure that all events have been consumed so + // https://github.com/flutter/flutter/issues/40863 won't happen again. + ASSERT_EQ(events_consumed_at_frame.back(), num_events); } TEST_F(ShellTest, MissAtMostOneFrameForIrregularInputEvents) { diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 3ede4058be9a6..42925e3913ad7 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -113,6 +113,29 @@ void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { latch.Wait(); } +void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { + fml::AutoResetWaitableEvent latch; + shell->GetTaskRunners().GetPlatformTaskRunner()->PostTask( + [shell, &will_draw_new_frame, &latch] { + // The following UI task ensures that all previous UI tasks are flushed. + fml::AutoResetWaitableEvent ui_latch; + shell->GetTaskRunners().GetUITaskRunner()->PostTask( + [&ui_latch, &will_draw_new_frame]() { + will_draw_new_frame = true; + ui_latch.Signal(); + }); + + ShellTestPlatformView* test_platform_view = + static_cast(shell->GetPlatformView().get()); + do { + test_platform_view->SimulateVSync(); + } while (ui_latch.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1))); + + latch.Signal(); + }); + latch.Wait(); +} + void ShellTest::PumpOneFrame(Shell* shell) { // Set viewport to nonempty, and call Animator::BeginFrame to make the layer // tree pipeline nonempty. Without either of this, the layer tree below @@ -232,6 +255,42 @@ void ShellTest::AddNativeCallback(std::string name, native_resolver_->AddNativeCallback(std::move(name), callback); } +void ShellTestVsyncClock::SimulateVSync() { + std::scoped_lock lock(mutex_); + if (vsync_issued_ >= vsync_promised_.size()) { + vsync_promised_.emplace_back(); + } + FML_CHECK(vsync_issued_ < vsync_promised_.size()); + vsync_promised_[vsync_issued_].set_value(vsync_issued_); + vsync_issued_ += 1; +} + +std::future ShellTestVsyncClock::NextVSync() { + std::scoped_lock lock(mutex_); + vsync_promised_.emplace_back(); + return vsync_promised_.back().get_future(); +} + +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()); }); +} + ShellTestPlatformView::ShellTestPlatformView(PlatformView::Delegate& delegate, TaskRunners task_runners) : PlatformView(delegate, std::move(task_runners)), @@ -239,6 +298,14 @@ ShellTestPlatformView::ShellTestPlatformView(PlatformView::Delegate& delegate, ShellTestPlatformView::~ShellTestPlatformView() = default; +std::unique_ptr ShellTestPlatformView::CreateVSyncWaiter() { + return std::make_unique(task_runners_, vsync_clock_); +} + +void ShellTestPlatformView::SimulateVSync() { + vsync_clock_.SimulateVSync(); +} + // |PlatformView| std::unique_ptr ShellTestPlatformView::CreateRenderingSurface() { return std::make_unique(this, true); diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index ae6c92560fa09..6bc249da577d0 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -9,6 +9,7 @@ #include "flutter/common/settings.h" #include "flutter/fml/macros.h" +#include "flutter/fml/synchronization/thread_annotations.h" #include "flutter/lib/ui/window/platform_message.h" #include "flutter/shell/common/run_configuration.h" #include "flutter/shell/common/shell.h" @@ -43,6 +44,10 @@ class ShellTest : public ThreadTest { static void RunEngine(Shell* shell, RunConfiguration configuration); static void RestartEngine(Shell* shell, RunConfiguration configuration); + /// Issue as many VSYNC as needed to flush the UI tasks so far, and reset + /// the `will_draw_new_frame` to true. + static void VSyncFlush(Shell* shell, bool& will_draw_new_frame); + static void PumpOneFrame(Shell* shell); static void DispatchFakePointerData(Shell* shell); @@ -73,6 +78,32 @@ class ShellTest : public ThreadTest { void SetSnapshotsAndAssets(Settings& settings); }; +class ShellTestVsyncClock { + public: + /// Simulate that a vsync signal is triggered. + void SimulateVSync(); + + /// A future that will return the index the next vsync signal. + std::future NextVSync(); + + private: + std::mutex mutex_; + std::vector> vsync_promised_ FML_GUARDED_BY(mutex_); + size_t vsync_issued_ FML_GUARDED_BY(mutex_) = 0; +}; + +class ShellTestVsyncWaiter : public VsyncWaiter { + public: + ShellTestVsyncWaiter(TaskRunners task_runners, ShellTestVsyncClock& clock) + : VsyncWaiter(std::move(task_runners)), clock_(clock) {} + + protected: + void AwaitVSync() override; + + private: + ShellTestVsyncClock& clock_; +}; + class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { public: ShellTestPlatformView(PlatformView::Delegate& delegate, @@ -80,12 +111,17 @@ class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { ~ShellTestPlatformView() override; + void SimulateVSync(); + private: TestGLSurface gl_surface_; // |PlatformView| std::unique_ptr CreateRenderingSurface() override; + // |PlatformView| + std::unique_ptr CreateVSyncWaiter() override; + // |PlatformView| PointerDataDispatcherMaker GetDispatcherMaker() override; @@ -107,6 +143,8 @@ class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { // |GPUSurfaceGLDelegate| ExternalViewEmbedder* GetExternalViewEmbedder() override; + ShellTestVsyncClock vsync_clock_; + FML_DISALLOW_COPY_AND_ASSIGN(ShellTestPlatformView); }; diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index b2efafa8c0079..628dd87f54ad5 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -56,6 +56,8 @@ void VsyncWaiter::AsyncWaitForVsync(Callback callback) { } void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { + FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); + if (!callback) { return; } From cc1f9edc24f33a08ce2ffef1f7f5c8f9b0e9516a Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 23 Sep 2019 18:02:42 -0700 Subject: [PATCH 4/6] Use default vsync waiter in previous tests --- shell/common/input_events_unittests.cc | 2 +- shell/common/shell_test.cc | 25 ++++++++++++++++--------- shell/common/shell_test.h | 14 +++++++++----- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/shell/common/input_events_unittests.cc b/shell/common/input_events_unittests.cc index 79d3baa1d5b4a..69688cce4a589 100644 --- a/shell/common/input_events_unittests.cc +++ b/shell/common/input_events_unittests.cc @@ -51,7 +51,7 @@ static void TestSimulatedInputEvents( bool restart_engine = false) { ///// Begin constructing shell /////////////////////////////////////////////// auto settings = fixture->CreateSettingsForFixture(); - std::unique_ptr shell = fixture->CreateShell(settings); + std::unique_ptr shell = fixture->CreateShell(settings, true); auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("onPointerDataPacketMain"); diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 42925e3913ad7..e498629d601fd 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -215,17 +215,20 @@ TaskRunners ShellTest::GetTaskRunnersForFixture() { }; } -std::unique_ptr ShellTest::CreateShell(Settings settings) { - return CreateShell(std::move(settings), GetTaskRunnersForFixture()); +std::unique_ptr ShellTest::CreateShell(Settings settings, + bool simulate_vsync) { + return CreateShell(std::move(settings), GetTaskRunnersForFixture(), + simulate_vsync); } std::unique_ptr ShellTest::CreateShell(Settings settings, - TaskRunners task_runners) { + TaskRunners task_runners, + bool simulate_vsync) { return Shell::Create( task_runners, settings, - [](Shell& shell) { - return std::make_unique(shell, - shell.GetTaskRunners()); + [simulate_vsync](Shell& shell) { + return std::make_unique( + shell, shell.GetTaskRunners(), simulate_vsync); }, [](Shell& shell) { return std::make_unique(shell, shell.GetTaskRunners()); @@ -292,14 +295,18 @@ void ShellTestVsyncWaiter::AwaitVSync() { } ShellTestPlatformView::ShellTestPlatformView(PlatformView::Delegate& delegate, - TaskRunners task_runners) + TaskRunners task_runners, + bool simulate_vsync) : PlatformView(delegate, std::move(task_runners)), - gl_surface_(SkISize::Make(800, 600)) {} + gl_surface_(SkISize::Make(800, 600)), + simulate_vsync_(simulate_vsync) {} ShellTestPlatformView::~ShellTestPlatformView() = default; std::unique_ptr ShellTestPlatformView::CreateVSyncWaiter() { - return std::make_unique(task_runners_, vsync_clock_); + return simulate_vsync_ ? std::make_unique(task_runners_, + vsync_clock_) + : PlatformView::CreateVSyncWaiter(); } void ShellTestPlatformView::SimulateVSync() { diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index 6bc249da577d0..01949661ea3a1 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -29,9 +29,11 @@ class ShellTest : public ThreadTest { ~ShellTest(); Settings CreateSettingsForFixture(); - std::unique_ptr CreateShell(Settings settings); std::unique_ptr CreateShell(Settings settings, - TaskRunners task_runners); + bool simulate_vsync = false); + std::unique_ptr CreateShell(Settings settings, + TaskRunners task_runners, + bool simulate_vsync = false); TaskRunners GetTaskRunnersForFixture(); void SendEnginePlatformMessage(Shell* shell, @@ -107,7 +109,8 @@ class ShellTestVsyncWaiter : public VsyncWaiter { class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { public: ShellTestPlatformView(PlatformView::Delegate& delegate, - TaskRunners task_runners); + TaskRunners task_runners, + bool simulate_vsync = false); ~ShellTestPlatformView() override; @@ -116,6 +119,9 @@ class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { private: TestGLSurface gl_surface_; + bool simulate_vsync_ = false; + ShellTestVsyncClock vsync_clock_; + // |PlatformView| std::unique_ptr CreateRenderingSurface() override; @@ -143,8 +149,6 @@ class ShellTestPlatformView : public PlatformView, public GPUSurfaceGLDelegate { // |GPUSurfaceGLDelegate| ExternalViewEmbedder* GetExternalViewEmbedder() override; - ShellTestVsyncClock vsync_clock_; - FML_DISALLOW_COPY_AND_ASSIGN(ShellTestPlatformView); }; From e62f996037fa51afe9c6dbd063fee5faf9bce365 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Wed, 25 Sep 2019 16:37:38 -0700 Subject: [PATCH 5/6] Fixes for Aaron's comments. --- shell/common/animator.h | 3 +++ shell/common/engine.cc | 2 +- shell/common/engine.h | 3 ++- shell/common/fixtures/shell_test.dart | 3 --- shell/common/platform_view.h | 2 +- shell/common/pointer_data_dispatcher.h | 12 ++++++++---- shell/common/shell_test.cc | 8 ++++---- shell/common/vsync_waiter.cc | 2 ++ shell/common/vsync_waiter.h | 3 +++ 9 files changed, 24 insertions(+), 14 deletions(-) diff --git a/shell/common/animator.h b/shell/common/animator.h index 7efe79ac5d80c..8d1fbedc4af00 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -52,6 +52,9 @@ class Animator final { void Render(std::unique_ptr layer_tree); + /// Add a secondary callback for the next vsync. + /// + /// See also |PointerDataDispatcher::ScheduleSecondaryVsyncCallback|. void ScheduleSecondaryVsyncCallback(std::function callback); void Start(); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 517479edcd1af..ce2b7a259da4c 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -36,7 +36,7 @@ static constexpr char kSettingsChannel[] = "flutter/settings"; static constexpr char kIsolateChannel[] = "flutter/isolate"; Engine::Engine(Delegate& delegate, - PointerDataDispatcherMaker& dispatcher_maker, + const PointerDataDispatcherMaker& dispatcher_maker, DartVM& vm, fml::RefPtr isolate_snapshot, fml::RefPtr shared_snapshot, diff --git a/shell/common/engine.h b/shell/common/engine.h index a913d27e3e16d..ae1ab081ee5b2 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -273,7 +273,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { /// GPU. /// Engine(Delegate& delegate, - PointerDataDispatcherMaker& dispatcher_maker, + const PointerDataDispatcherMaker& dispatcher_maker, DartVM& vm, fml::RefPtr isolate_snapshot, fml::RefPtr shared_snapshot, @@ -713,6 +713,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { void DoDispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) override; + // |PointerDataDispatcher::Delegate| void ScheduleSecondaryVsyncCallback(std::function callback) override; private: diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 9ba950d83b57b..05f4782fc64ee 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -38,9 +38,6 @@ void onPointerDataPacketMain() { window.onPointerDataPacket = (PointerDataPacket packet) { nativeOnPointerDataPacket(); }; - window.onBeginFrame = (Duration beginTime) { - nativeOnBeginFrame(beginTime.inMicroseconds); - }; } @pragma('vm:entry-point') diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 9bbc0a6baf935..ad4170f776a79 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -421,7 +421,7 @@ class PlatformView { //-------------------------------------------------------------------------- /// @brief Returns a platform-specific PointerDataDispatcherMaker so the - /// `Engien` can construct the PointerDataPacketDispatcher based + /// `Engine` can construct the PointerDataPacketDispatcher based /// on platforms. virtual PointerDataDispatcherMaker GetDispatcherMaker(); diff --git a/shell/common/pointer_data_dispatcher.h b/shell/common/pointer_data_dispatcher.h index 6fb2e9a422672..06db4d501be35 100644 --- a/shell/common/pointer_data_dispatcher.h +++ b/shell/common/pointer_data_dispatcher.h @@ -35,7 +35,7 @@ class PointerDataDispatcher; /// (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 +/// platform thread, and sends it to the UI thread for the final /// construction of the `PointerDataDispatcher`. class PointerDataDispatcher { public: @@ -49,10 +49,14 @@ class PointerDataDispatcher { //---------------------------------------------------------------------------- /// @brief Schedule a secondary callback to be executed right after the - /// main `Animator::RequestFrame` callback. + /// main `VsyncWaiter::AsyncWaitForVsync` callback (which is added + /// by `Animator::RequestFrame`). /// - /// If there is no RequestFrame callback, this secondary callback - /// will still be executed at vsync. + /// Like the callback in `AsyncWaitForVsync`, this callback is + /// only scheduled to be called once, and it's supposed to be + /// called in the UI thread. If there is no AsyncWaitForVsync + /// callback (`Animator::RequestFrame` is not called), this + /// secondary callback will still be executed at vsync. /// /// This callback is used to provide the vsync signal needed by /// `SmoothPointerDataDispatcher`. diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index e498629d601fd..470c63557f52b 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -102,15 +102,15 @@ void ShellTest::RunEngine(Shell* shell, RunConfiguration configuration) { } void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { - fml::AutoResetWaitableEvent latch; + std::promise finished; fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetUITaskRunner(), - [shell, &latch, &configuration]() { + [shell, &finished, &configuration]() { bool restarted = shell->engine_->Restart(std::move(configuration)); ASSERT_TRUE(restarted); - latch.Signal(); + finished.set_value(true); }); - latch.Wait(); + finished.get_future().wait(); } void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index 628dd87f54ad5..fb31330b9388d 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -68,6 +68,8 @@ void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { std::scoped_lock lock(callback_mutex_); if (secondary_callback_) { // Multiple schedules must result in a single callback per frame interval. + TRACE_EVENT_INSTANT0("flutter", + "MultipleCallsToSecondaryVsyncInFrameInterval"); return; } secondary_callback_ = std::move(callback); diff --git a/shell/common/vsync_waiter.h b/shell/common/vsync_waiter.h index 6b7bcd35eaf0a..7dd1dedfba800 100644 --- a/shell/common/vsync_waiter.h +++ b/shell/common/vsync_waiter.h @@ -26,6 +26,9 @@ class VsyncWaiter : public std::enable_shared_from_this { void AsyncWaitForVsync(Callback callback); + /// Add a secondary callback for the next vsync. + /// + /// See also |PointerDataDispatcher::ScheduleSecondaryVsyncCallback|. void ScheduleSecondaryCallback(std::function callback); static constexpr float kUnknownRefreshRateFPS = 0.0; From 11c568022892c40738506f4f6569f977af28fc8c Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 27 Sep 2019 11:27:26 -0700 Subject: [PATCH 6/6] More fixes for comments --- shell/common/animator.cc | 2 +- shell/common/animator.h | 18 ++++++++-- shell/common/engine.cc | 2 +- shell/common/engine.h | 12 +++---- shell/common/pointer_data_dispatcher.cc | 3 ++ shell/common/pointer_data_dispatcher.h | 25 +++++++------- shell/common/shell_test.cc | 45 +++++++++++++------------ shell/common/vsync_waiter.cc | 4 +-- shell/common/vsync_waiter.h | 6 ++-- 9 files changed, 67 insertions(+), 50 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 7e3f63eff91aa..b0ef6fca098f2 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -244,7 +244,7 @@ void Animator::AwaitVSync() { delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_); } -void Animator::ScheduleSecondaryVsyncCallback(std::function callback) { +void Animator::ScheduleSecondaryVsyncCallback(fml::closure callback) { waiter_->ScheduleSecondaryCallback(std::move(callback)); } diff --git a/shell/common/animator.h b/shell/common/animator.h index 8d1fbedc4af00..dddc70b145b46 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -52,10 +52,22 @@ class Animator final { void Render(std::unique_ptr layer_tree); - /// Add a secondary callback for the next vsync. + //-------------------------------------------------------------------------- + /// @brief Schedule a secondary callback to be executed right after the + /// main `VsyncWaiter::AsyncWaitForVsync` callback (which is added + /// by `Animator::RequestFrame`). /// - /// See also |PointerDataDispatcher::ScheduleSecondaryVsyncCallback|. - void ScheduleSecondaryVsyncCallback(std::function callback); + /// Like the callback in `AsyncWaitForVsync`, this callback is + /// only scheduled to be called once, and it's supposed to be + /// called in the UI thread. If there is no AsyncWaitForVsync + /// callback (`Animator::RequestFrame` is not called), this + /// secondary callback will still be executed at vsync. + /// + /// This callback is used to provide the vsync signal needed by + /// `SmoothPointerDataDispatcher`. + /// + /// @see `PointerDataDispatcher::ScheduleSecondaryVsyncCallback`. + void ScheduleSecondaryVsyncCallback(fml::closure callback); void Start(); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index ce2b7a259da4c..f106c662dafff 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -474,7 +474,7 @@ void Engine::DoDispatchPacket(std::unique_ptr packet, } } -void Engine::ScheduleSecondaryVsyncCallback(std::function callback) { +void Engine::ScheduleSecondaryVsyncCallback(fml::closure callback) { animator_->ScheduleSecondaryVsyncCallback(std::move(callback)); } diff --git a/shell/common/engine.h b/shell/common/engine.h index ae1ab081ee5b2..2e4fd41964bb2 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -236,11 +236,11 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { /// 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 - /// `PlatformView` for engine to create the - /// pointer data dispatcher. Similar to other - /// engine resources, this dispatcher_maker and - /// its returned dispatcher is only safe to be + /// @param dispatcher_maker The callback provided by `PlatformView` for + /// engine to create the pointer data + /// dispatcher. Similar to other engine + /// resources, this dispatcher_maker and its + /// returned dispatcher is only safe to be /// called from the UI thread. /// @param vm An instance of the running Dart VM. /// @param[in] isolate_snapshot The snapshot used to create the root @@ -714,7 +714,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { uint64_t trace_flow_id) override; // |PointerDataDispatcher::Delegate| - void ScheduleSecondaryVsyncCallback(std::function callback) override; + void ScheduleSecondaryVsyncCallback(fml::closure callback) override; private: Engine::Delegate& delegate_; diff --git a/shell/common/pointer_data_dispatcher.cc b/shell/common/pointer_data_dispatcher.cc index 508fc0c09c113..8341ecec10ab2 100644 --- a/shell/common/pointer_data_dispatcher.cc +++ b/shell/common/pointer_data_dispatcher.cc @@ -8,6 +8,9 @@ namespace flutter { PointerDataDispatcher::~PointerDataDispatcher() = default; DefaultPointerDataDispatcher::~DefaultPointerDataDispatcher() = default; + +SmoothPointerDataDispatcher::SmoothPointerDataDispatcher(Delegate& delegate) + : DefaultPointerDataDispatcher(delegate), weak_factory_(this) {} SmoothPointerDataDispatcher::~SmoothPointerDataDispatcher() = default; void DefaultPointerDataDispatcher::DispatchPacket( diff --git a/shell/common/pointer_data_dispatcher.h b/shell/common/pointer_data_dispatcher.h index 06db4d501be35..d67a5b6fe7ff3 100644 --- a/shell/common/pointer_data_dispatcher.h +++ b/shell/common/pointer_data_dispatcher.h @@ -47,21 +47,20 @@ class PointerDataDispatcher { virtual void DoDispatchPacket(std::unique_ptr packet, uint64_t trace_flow_id) = 0; - //---------------------------------------------------------------------------- + //-------------------------------------------------------------------------- /// @brief Schedule a secondary callback to be executed right after the /// main `VsyncWaiter::AsyncWaitForVsync` callback (which is added /// by `Animator::RequestFrame`). /// /// Like the callback in `AsyncWaitForVsync`, this callback is - /// only scheduled to be called once, and it's supposed to be - /// called in the UI thread. If there is no AsyncWaitForVsync - /// callback (`Animator::RequestFrame` is not called), this - /// secondary callback will still be executed at vsync. + /// only scheduled to be called once, and it will be called in the + /// UI thread. If there is no AsyncWaitForVsync callback + /// (`Animator::RequestFrame` is not called), this secondary + /// callback will still be executed at vsync. /// /// This callback is used to provide the vsync signal needed by /// `SmoothPointerDataDispatcher`. - virtual void ScheduleSecondaryVsyncCallback( - std::function callback) = 0; + virtual void ScheduleSecondaryVsyncCallback(fml::closure callback) = 0; }; //---------------------------------------------------------------------------- @@ -97,10 +96,11 @@ class DefaultPointerDataDispatcher : public PointerDataDispatcher { }; //------------------------------------------------------------------------------ -/// 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. +/// A dispatcher that may temporarily store and defer the last received +/// PointerDataPacket if multiple packets are received in one VSYNC. The +/// deferred packet will be sent in the next vsync in order to smooth out the +/// events. This filters out irregular input events delivery to provide a smooth +/// scroll on iPhone X/Xs. /// /// It works as follows: /// @@ -135,8 +135,7 @@ class DefaultPointerDataDispatcher : public PointerDataDispatcher { /// See also input_events_unittests.cc where we test all our claims above. class SmoothPointerDataDispatcher : public DefaultPointerDataDispatcher { public: - SmoothPointerDataDispatcher(Delegate& delegate) - : DefaultPointerDataDispatcher(delegate), weak_factory_(this) {} + SmoothPointerDataDispatcher(Delegate& delegate); // |PointerDataDispatcer| void DispatchPacket(std::unique_ptr packet, diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 470c63557f52b..64ef4da369cf3 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #define FML_USED_ON_EMBEDDER #include "flutter/shell/common/shell_test.h" @@ -102,15 +103,13 @@ void ShellTest::RunEngine(Shell* shell, RunConfiguration configuration) { } void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { - std::promise finished; + std::promise restarted; fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetUITaskRunner(), - [shell, &finished, &configuration]() { - bool restarted = shell->engine_->Restart(std::move(configuration)); - ASSERT_TRUE(restarted); - finished.set_value(true); + [shell, &restarted, &configuration]() { + restarted.set_value(shell->engine_->Restart(std::move(configuration))); }); - finished.get_future().wait(); + ASSERT_TRUE(restarted.get_future().get()); } void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { @@ -277,21 +276,25 @@ std::future ShellTestVsyncClock::NextVSync() { 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()); }); + + auto async_wait = 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 resets 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()); + }); + }); } ShellTestPlatformView::ShellTestPlatformView(PlatformView::Delegate& delegate, diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index fb31330b9388d..c191a13490006 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -55,7 +55,7 @@ void VsyncWaiter::AsyncWaitForVsync(Callback callback) { AwaitVSync(); } -void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { +void VsyncWaiter::ScheduleSecondaryCallback(fml::closure callback) { FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); if (!callback) { @@ -85,7 +85,7 @@ void VsyncWaiter::ScheduleSecondaryCallback(std::function callback) { void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, fml::TimePoint frame_target_time) { Callback callback; - std::function secondary_callback; + fml::closure secondary_callback; { std::scoped_lock lock(callback_mutex_); diff --git a/shell/common/vsync_waiter.h b/shell/common/vsync_waiter.h index 7dd1dedfba800..e93b730fe7fab 100644 --- a/shell/common/vsync_waiter.h +++ b/shell/common/vsync_waiter.h @@ -29,7 +29,7 @@ class VsyncWaiter : public std::enable_shared_from_this { /// Add a secondary callback for the next vsync. /// /// See also |PointerDataDispatcher::ScheduleSecondaryVsyncCallback|. - void ScheduleSecondaryCallback(std::function callback); + void ScheduleSecondaryCallback(fml::closure callback); static constexpr float kUnknownRefreshRateFPS = 0.0; @@ -50,7 +50,7 @@ class VsyncWaiter : public std::enable_shared_from_this { // Implementations are meant to override this method and arm their vsync // latches when in response to this invocation. On vsync, they are meant to // invoke the |FireCallback| method once (and only once) with the appropriate - // arguments. + // arguments. This method should not block the current thread. virtual void AwaitVSync() = 0; void FireCallback(fml::TimePoint frame_start_time, @@ -61,7 +61,7 @@ class VsyncWaiter : public std::enable_shared_from_this { Callback callback_ FML_GUARDED_BY(callback_mutex_); std::mutex secondary_callback_mutex_; - std::function secondary_callback_ FML_GUARDED_BY(callback_mutex_); + fml::closure secondary_callback_ FML_GUARDED_BY(callback_mutex_); FML_DISALLOW_COPY_AND_ASSIGN(VsyncWaiter); };