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

Add scheduleWarmUpFrame #50570

Merged
merged 28 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1950e72
Impl engine
dkwingsmt Feb 12, 2024
f55dfdf
Web
dkwingsmt Feb 12, 2024
7f76c5f
Rename to RequestWarmUpFrame and add web time
dkwingsmt Feb 13, 2024
31a5ea3
Change to scheduleWarmUpFrame and EndWarmUpFrame
dkwingsmt Feb 13, 2024
590287d
Comment
dkwingsmt Feb 13, 2024
40b269b
Doc
dkwingsmt Feb 13, 2024
2d70a0f
Simplify web implementation
dkwingsmt Feb 14, 2024
4af9f06
Fix comment
dkwingsmt Feb 14, 2024
bfe5570
More doc
dkwingsmt Feb 14, 2024
21439c4
Fix doc
dkwingsmt Feb 14, 2024
1bd21db
More doc
dkwingsmt Feb 14, 2024
4f41658
Fix linter
dkwingsmt Feb 14, 2024
eeac064
Fix comment
dkwingsmt Feb 14, 2024
9c5bce4
Add test
dkwingsmt Feb 15, 2024
cdcf71a
Fix test
dkwingsmt Feb 15, 2024
7ab7d8e
Better test
dkwingsmt Feb 15, 2024
afbcfae
Merge remote-tracking branch 'origin/main' into force-sync-frame
dkwingsmt Feb 15, 2024
7381087
Simplify test and add platformdispatcher test
dkwingsmt Feb 15, 2024
05d3d2d
Better structure
dkwingsmt Feb 15, 2024
06957bb
Better name
dkwingsmt Feb 15, 2024
68953c0
Merge branch 'main' into force-sync-frame
dkwingsmt Feb 15, 2024
a9b7511
Merge branch 'main' into force-sync-frame
dkwingsmt Feb 20, 2024
8731e47
Add web platform dispatcher test
dkwingsmt Feb 20, 2024
cdeffd9
Merge branch 'main' into force-sync-frame
dkwingsmt Feb 20, 2024
5d9f48b
Web comments
dkwingsmt Feb 21, 2024
1589c59
Merge remote-tracking branch 'origin/main' into force-sync-frame
dkwingsmt Feb 21, 2024
556984f
Fix test
dkwingsmt Feb 21, 2024
2b6d3b2
Revert timer run change
dkwingsmt Feb 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/ui/dart_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ typedef CanvasPath Path;
V(NativeStringAttribute::initSpellOutStringAttribute) \
V(PlatformConfigurationNativeApi::DefaultRouteName) \
V(PlatformConfigurationNativeApi::ScheduleFrame) \
V(PlatformConfigurationNativeApi::EndWarmUpFrame) \
V(PlatformConfigurationNativeApi::Render) \
V(PlatformConfigurationNativeApi::UpdateSemantics) \
V(PlatformConfigurationNativeApi::SetNeedsReportTimings) \
Expand Down
36 changes: 36 additions & 0 deletions lib/ui/platform_dispatcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -801,11 +801,47 @@ class PlatformDispatcher {
///
/// * [SchedulerBinding], the Flutter framework class which manages the
/// scheduling of frames.
/// * [scheduleWarmUpFrame], which should only be used to schedule warm up
/// frames.
void scheduleFrame() => _scheduleFrame();

@Native<Void Function()>(symbol: 'PlatformConfigurationNativeApi::ScheduleFrame')
external static void _scheduleFrame();

/// Schedule a frame to run as soon as possible, rather than waiting for the
/// engine to request a frame in response to a system "Vsync" signal.
///
/// The application can call this method as soon as it starts up so that the
/// first frame (which is likely to be quite expensive) can start a few extra
/// milliseconds earlier. Using it in other situations might lead to
/// unintended results, such as screen tearing. Depending on platforms and
/// situations, the warm up frame might or might not be actually rendered onto
/// the screen.
///
/// For more introduction to the warm up frame, see
/// [SchedulerBinding.scheduleWarmUpFrame].
///
/// This method uses the provided callbacks as the begin frame callback and
/// the draw frame callback instead of [onBeginFrame] and [onDrawFrame].
///
/// See also:
///
/// * [SchedulerBinding.scheduleWarmUpFrame], which uses this method, and
/// introduces the warm up frame in more details.
/// * [scheduleFrame], which schedules the frame at the next appropriate
/// opportunity and should be used to render regular frames.
void scheduleWarmUpFrame({required VoidCallback beginFrame, required VoidCallback drawFrame}) {
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is meant for first frame only, why not call it scheduleFirstFrame?

Copy link
Contributor Author

@dkwingsmt dkwingsmt Feb 21, 2024

Choose a reason for hiding this comment

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

The biggest reason, in my opinion, is that this frame is not guaranteed to be rendered, for example if Animator::BeginFrame is not called before, or on Web (if you agree with my other comment). Therefore this might not be the first frame.

Besides, I don't want to give developers the impression that scheduling the first frame using scheduleFrame is wrong. The scheduleWarmUpFrame really is for warming up. An app doesn't need warm-ups to perform correctly, but will perform better. It's probably the same reason why SchedulerBinding.scheduleWarmUpFrame is not called scheduleFirstFrame.

Copy link
Member

Choose a reason for hiding this comment

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

In addition to what Tong said: The warmup frame is also used in hot reload scenarios where it isn't the first frame of the app.

Copy link
Contributor

Choose a reason for hiding this comment

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

The semantics of this method sound pretty complicated. I guess warm up is fine then. Should we have some assertions in the engine for this method? For example, should we make sure it's only called for the first frame, and is never called after a scheduleFrame is called? Simply exposing this as a sibling to scheduleFrame seems to contain footguns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if it's possible, since SchedulerBinding.scheduleWarmUpFrame is also called right after hot reloading. I think it's better leave it to the app, i.e. the framework.

// We use timers here to ensure that microtasks flush in between.
Timer.run(beginFrame);
Timer.run(() {
drawFrame();
_endWarmUpFrame();
});
}

@Native<Void Function()>(symbol: 'PlatformConfigurationNativeApi::EndWarmUpFrame')
external static void _endWarmUpFrame();

/// Additional accessibility features that may be enabled by the platform.
AccessibilityFeatures get accessibilityFeatures => _configuration.accessibilityFeatures;

Expand Down
5 changes: 5 additions & 0 deletions lib/ui/window/platform_configuration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,11 @@ void PlatformConfigurationNativeApi::ScheduleFrame() {
UIDartState::Current()->platform_configuration()->client()->ScheduleFrame();
}

void PlatformConfigurationNativeApi::EndWarmUpFrame() {
UIDartState::ThrowIfUIOperationsProhibited();
UIDartState::Current()->platform_configuration()->client()->EndWarmUpFrame();
}

void PlatformConfigurationNativeApi::UpdateSemantics(SemanticsUpdate* update) {
UIDartState::ThrowIfUIOperationsProhibited();
UIDartState::Current()->platform_configuration()->client()->UpdateSemantics(
Expand Down
9 changes: 9 additions & 0 deletions lib/ui/window/platform_configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ class PlatformConfigurationClient {
///
virtual void ScheduleFrame() = 0;

//--------------------------------------------------------------------------
/// @brief Called when a warm up frame has ended.
///
/// For more introduction, see `Animator::EndWarmUpFrame`.
///
virtual void EndWarmUpFrame() = 0;

//--------------------------------------------------------------------------
/// @brief Updates the client's rendering on the GPU with the newly
/// provided Scene.
Expand Down Expand Up @@ -557,6 +564,8 @@ class PlatformConfigurationNativeApi {

static void ScheduleFrame();

static void EndWarmUpFrame();

static void Render(int64_t view_id,
Scene* scene,
double width,
Expand Down
2 changes: 2 additions & 0 deletions lib/web_ui/lib/platform_dispatcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ abstract class PlatformDispatcher {

void scheduleFrame();

void scheduleWarmUpFrame({required VoidCallback beginFrame, required VoidCallback drawFrame});

AccessibilityFeatures get accessibilityFeatures;

VoidCallback? get onAccessibilityFeaturesChanged;
Expand Down
3 changes: 2 additions & 1 deletion lib/web_ui/lib/src/engine/initialization.dart
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ Future<void> initializeEngineServices({
// TODO(yjbanov): technically Flutter flushes microtasks between
// onBeginFrame and onDrawFrame. We don't, which hasn't
// been an issue yet, but eventually we'll have to
// implement it properly.
// implement it properly. (Also see the to-do in
// `EnginePlatformDispatcher.scheduleWarmUpFrame`).
EnginePlatformDispatcher.instance.invokeOnDrawFrame();
}
});
Expand Down
13 changes: 13 additions & 0 deletions lib/web_ui/lib/src/engine/platform_dispatcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,19 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher {
scheduleFrameCallback!();
}

@override
void scheduleWarmUpFrame({required ui.VoidCallback beginFrame, required ui.VoidCallback drawFrame}) {
Timer.run(beginFrame);
// We use timers here to ensure that microtasks flush in between.
//
// TODO(dkwingsmt): This logic was moved from the framework and is different
// from how Web renders a regular frame, which doesn't flush microtasks
// between the callbacks at all (see `initializeEngineServices`). We might
// want to change this. See the to-do in `initializeEngineServices` and
// https://github.com/flutter/engine/pull/50570#discussion_r1496671676
Timer.run(drawFrame);
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
}

/// Updates the application's rendering on the GPU with the newly provided
/// [Scene]. This function must be called within the scope of the
/// [onBeginFrame] or [onDrawFrame] callbacks being invoked. If this function
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,23 @@ void testMain() {
dispatcher.dispose();
expect(dispatcher.accessibilityPlaceholder.isConnected, isFalse);
});

test('scheduleWarmupFrame should call both callbacks', () async {
bool beginFrameCalled = false;
final Completer<void> drawFrameCalled = Completer<void>();
dispatcher.scheduleWarmUpFrame(beginFrame: () {
expect(drawFrameCalled.isCompleted, false);
expect(beginFrameCalled, false);
beginFrameCalled = true;
}, drawFrame: () {
expect(beginFrameCalled, true);
expect(drawFrameCalled.isCompleted, false);
drawFrameCalled.complete();
});
await drawFrameCalled.future;
expect(beginFrameCalled, true);
expect(drawFrameCalled.isCompleted, true);
});
});
}

Expand Down
5 changes: 5 additions & 0 deletions runtime/runtime_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,11 @@ void RuntimeController::ScheduleFrame() {
client_.ScheduleFrame();
}

// |PlatformConfigurationClient|
void RuntimeController::EndWarmUpFrame() {
client_.EndWarmUpFrame();
}

// |PlatformConfigurationClient|
void RuntimeController::Render(Scene* scene, double width, double height) {
// TODO(dkwingsmt): Currently only supports a single window.
Expand Down
3 changes: 3 additions & 0 deletions runtime/runtime_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,9 @@ class RuntimeController : public PlatformConfigurationClient {
// |PlatformConfigurationClient|
void ScheduleFrame() override;

// |PlatformConfigurationClient|
void EndWarmUpFrame() override;

// |PlatformConfigurationClient|
void Render(Scene* scene, double width, double height) override;

Expand Down
2 changes: 2 additions & 0 deletions runtime/runtime_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class RuntimeDelegate {

virtual void ScheduleFrame(bool regenerate_layer_trees = true) = 0;

virtual void EndWarmUpFrame() = 0;

virtual void Render(std::unique_ptr<flutter::LayerTree> layer_tree,
float device_pixel_ratio) = 0;

Expand Down
6 changes: 6 additions & 0 deletions shell/common/animator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,12 @@ void Animator::AwaitVSync() {
}
}

void Animator::EndWarmUpFrame() {
// Do nothing. The warm up frame does not need any additional work to end the
// frame for now. This will change once the pipeline supports multi-view.
// https://github.com/flutter/flutter/issues/142851
}

void Animator::ScheduleSecondaryVsyncCallback(uintptr_t id,
const fml::closure& callback) {
waiter_->ScheduleSecondaryCallback(id, callback);
Expand Down
16 changes: 16 additions & 0 deletions shell/common/animator.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,22 @@ class Animator final {

void RequestFrame(bool regenerate_layer_trees = true);

//--------------------------------------------------------------------------
/// @brief Tells the Animator that a warm up frame has ended.
///
/// In a warm up frame, `Animator::Render` is called out of vsync
/// tasks, and Animator requires an explicit end-of-frame call to
/// know when to send the layer trees to the pipeline.
///
/// This is different from regular frames, where Animator::Render is
/// always called within a vsync task, and Animator can send
/// the views at the end of the vsync task.
///
/// For more about warm up frames, see
/// `PlatformDispatcher.scheduleWarmUpFrame`.
///
void EndWarmUpFrame();

//--------------------------------------------------------------------------
/// @brief Tells the Animator that this frame needs to render another view.
///
Expand Down
4 changes: 4 additions & 0 deletions shell/common/engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,10 @@ void Engine::ScheduleFrame(bool regenerate_layer_trees) {
animator_->RequestFrame(regenerate_layer_trees);
}

void Engine::EndWarmUpFrame() {
animator_->EndWarmUpFrame();
}

void Engine::Render(std::unique_ptr<flutter::LayerTree> layer_tree,
float device_pixel_ratio) {
if (!layer_tree) {
Expand Down
3 changes: 3 additions & 0 deletions shell/common/engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,9 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate {
/// tree.
void ScheduleFrame() { ScheduleFrame(true); }

// |RuntimeDelegate|
void EndWarmUpFrame() override;

// |RuntimeDelegate|
FontCollection& GetFontCollection() override;

Expand Down
Loading