Add scheduleWarmUpFrame #50570

dkwingsmt · 2024-02-12T22:30:28Z

This PR adds PlatformDispatcher.scheduleWarmUpFrame.

This PR is needed for the follow up changes:

The framework will switch to using this function to render warmup frames in Render the warm up frame in a proper rendering process flutter#143290.
Then the engine will finally be able to switch to multiview pipeline with no regression on startup timing in Reland 3: Multiview pipeline #49950.

For why the warm up frame must involve the engine to render, see flutter/flutter#142851.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

lib/ui/platform_dispatcher.dart

loic-sharma · 2024-02-12T23:44:11Z

lib/web_ui/lib/src/engine/initialization.dart

      });
    }
  };
+  warmUpFrameCallback = () {
+    // TODO(dkwingsmt): Can we give it some time value?
+    renderFrame(0);


Consider making this a named argument for readability

loic-sharma · 2024-02-12T23:45:49Z

shell/common/animator.h

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

  void RequestFrame(bool regenerate_layer_trees = true);

+  void ForceSyncFrame();


Please add a quick comment here

shell/common/animator.cc

lib/ui/platform_dispatcher.dart

dnfield · 2024-02-13T03:06:27Z

One alternative that may be even simpler is to encode the way the framework works here into the engine itself.

In other words, rather than allowing the framework to request this, we just fire off the first request for rendering ASAP instead of waiting for the vsync handler to be hooked up.

That may avoid some of the confusion around how to use this API. At any rate, we should not create an API that makes application authors think they can call this whenever they want a "synchronous frame" to render.

dkwingsmt · 2024-02-13T05:42:54Z

One alternative that may be even simpler is to encode the way the framework works here into the engine itself.

In other words, rather than allowing the framework to request this, we just fire off the first request for rendering ASAP instead of waiting for the vsync handler to be hooked up.

That may avoid some of the confusion around how to use this API. At any rate, we should not create an API that makes application authors think they can call this whenever they want a "synchronous frame" to render.

Yeah I agree. During my discussion with @/goderbauer and @/loic-sharma we also wanted to make the name sound intimidating for people to not use it lightly.

I like your idea, but it's not very feasible since this requires that the app (not necessarily the framework; the app could use dart:ui directly) assigning the warm up frame callback, and that the engine knows when it's assigned.

Since the objection is strong, I'll change the API name.

jonahwilliams · 2024-02-13T17:51:38Z

My feedback from the previous PR that I did not give a chance to give: Like Dan, I'm concerned about the name + documentation. The warmup frame is a very special case, and I think the engine API that we added for this should be clear that it is for a special case, and not documented as a general purpose API.

jonahwilliams · 2024-02-13T17:56:34Z

Specifically, I think the name should include "warmup" in the name

dkwingsmt · 2024-02-14T00:57:19Z

I've overhauled this PR by adopting the two-callback approach suggested by @dnfield. This PR should be ready for review except for some unit tests for the new API.
@jonahwilliams @loic-sharma @goderbauer

The accompanying PRs have also be updated as well, for reference:

Render the warm up frame in a proper rendering process flutter#143290, which should be merged after this PR and make the framework use the new API.
Reland 3: Multiview pipeline #49950, which should be merged after the framework PR and make the pipeline multi-view.

dkwingsmt · 2024-02-15T21:06:35Z

@dnfield Thank you! I've added your test to the PR.

jonahwilliams

LGTM

lib/ui/platform_dispatcher.dart

yjbanov · 2024-02-20T23:14:51Z

lib/ui/platform_dispatcher.dart

+  ///    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}) {


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

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.

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.

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.

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.

yjbanov · 2024-02-20T23:16:20Z

lib/ui/platform_dispatcher.dart

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


Do we also want to use this when rendering the first frame in a new FlutterView? I imagine we'd want the new view to be rendered as fast as possible too.

That's a good point, although I imagine this would require more work. Can I open an issue to leave the optimization for later?

I don't think this optimization works for individual views. Because of the unified widget tree we can only produce frames for all views at once and cannot target individual views. This optimization only works for the app as a whole.

Nope, I was just hypothesizing. If we don't need it, then we don't need it. My gut feeling is that we don't need any of this on the web. So I'm actually a little sad that this API even exists.

lib/web_ui/lib/src/engine/platform_dispatcher.dart

lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart

yjbanov

LGTM. None of my comments are blocking.

…143911) flutter/engine@bf5c003...7eeb697 2024-02-22 tugorez@users.noreply.github.com Add view focus direction detection to flutter web. (flutter/engine#50843) 2024-02-22 matanlurey@users.noreply.github.com Add a similar `runIf` guard to `web_engine` as web framework. (flutter/engine#50846) 2024-02-22 dkwingsmt@users.noreply.github.com Add scheduleWarmUpFrame (flutter/engine#50570) 2024-02-22 matanlurey@users.noreply.github.com Remove/reduce unused or private methods and add tests to `SkiaGoldClient` (flutter/engine#50844) 2024-02-22 matanlurey@users.noreply.github.com Clean up contributing formatting, add a Skia gold callout (flutter/engine#50828) 2024-02-21 34871572+gmackall@users.noreply.github.com Make Android Studio be able to run android embedding unit tests out of the box (flutter/engine#50840) 2024-02-21 zanderso@users.noreply.github.com Shift some deps to //flutter/third_party (flutter/engine#50830) 2024-02-21 tugorez@users.noreply.github.com Make the view focus binding report focus transitions across elements. (flutter/engine#50610) 2024-02-21 chris@bracken.jp [macOS] Wrap FlutterEngineTest in autoreleasepool (flutter/engine#50832) 2024-02-21 jason-simmons@users.noreply.github.com Update the vulkan_glfw sample for the latest roll of vulkan-deps (flutter/engine#50839) 2024-02-21 skia-flutter-autoroll@skia.org Roll Skia from 8fa858855820 to 57490c8d257e (6 revisions) (flutter/engine#50833) 2024-02-21 zanderso@users.noreply.github.com Starts a .ci.yaml parser (flutter/engine#50783) 2024-02-21 skia-flutter-autoroll@skia.org Roll Dart SDK from f344e2266468 to 0f0f7400c38a (6 revisions) (flutter/engine#50837) 2024-02-21 737941+loic-sharma@users.noreply.github.com [Windows] Fix top-level message procedure order (flutter/engine#50797) 2024-02-21 john@johnmccutchan.com Tweak verbose log messages in ImageReaderSurfaceProducer (flutter/engine#50831) 2024-02-21 matanlurey@users.noreply.github.com Add a throw statement for imgtestAdd non 0 exit codes. (flutter/engine#50829) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

_This PR requires flutter/engine#50570 This PR uses the new `PlatformDispatcher.scheduleWarmUpFrame` API to render warm up frames. For why the warm up frame must involve the engine to render, see #142851.

This is the 3rd attempt to land multiview pipeline, following #47239. The pipeline now properly implements the required logic for `scheduleWarmUpFrame` to work in a multi-view setup, following the preparation in flutter/flutter#143290 and #50570. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

Reverts #49950 Initiated by: dkwingsmt Reason for reverting: Head redness ``` java.lang.RuntimeException: Timeout waiting for firstFrameLatch to signal at dev.flutter.scenarios.ExternalTextureFlutterActivity.waitUntilFlutterRendered(ExternalTextureFlutterActivity.java:98) at dev.flutter.scenariosui.ScreenshotUtil.capture(ScreenshotUtil.java:122) ``` Original PR Author: dkwingsmt Reviewed By: {loic-sharma} This change reverts the following previous change: Original Description: This is the 3rd attempt to land multiview pipeline, following #47239. The pipeline now properly implements the required logic for `scheduleWarmUpFrame` to work in a multi-view setup, following the preparation in flutter/flutter#143290 and #50570. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>

Impl engine

1950e72

github-actions bot added the platform-web Code specifically for the web engine label Feb 12, 2024

Web

f55dfdf

dkwingsmt force-pushed the force-sync-frame branch from 4d0e655 to f55dfdf Compare February 12, 2024 22:39

loic-sharma reviewed Feb 12, 2024

View reviewed changes

lib/ui/platform_dispatcher.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 12, 2024

View reviewed changes

lib/ui/platform_dispatcher.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 12, 2024

View reviewed changes

shell/common/animator.cc Outdated Show resolved Hide resolved

jonahwilliams self-requested a review February 13, 2024 01:02

dnfield suggested changes Feb 13, 2024

View reviewed changes

lib/ui/platform_dispatcher.dart Outdated Show resolved Hide resolved

lib/ui/platform_dispatcher.dart Outdated Show resolved Hide resolved

Rename to RequestWarmUpFrame and add web time

7f76c5f

dkwingsmt added 4 commits February 13, 2024 14:25

Change to scheduleWarmUpFrame and EndWarmUpFrame

31a5ea3

Comment

590287d

Doc

40b269b

Simplify web implementation

2d70a0f

dkwingsmt changed the title ~~Allow synchronously rendering a frame~~ Add scheduleWarmUpFrame Feb 14, 2024

dkwingsmt added 4 commits February 13, 2024 16:38

Fix comment

4af9f06

More doc

bfe5570

Fix doc

21439c4

More doc

1bd21db

dkwingsmt requested review from dnfield, loic-sharma and goderbauer February 14, 2024 00:57

dkwingsmt mentioned this pull request Feb 14, 2024

Render the warm up frame in a proper rendering process flutter/flutter#143290

Merged

8 tasks

Better name

06957bb

Merge branch 'main' into force-sync-frame

68953c0

dnfield approved these changes Feb 20, 2024

View reviewed changes

Merge branch 'main' into force-sync-frame

a9b7511

jonahwilliams approved these changes Feb 20, 2024

View reviewed changes

dkwingsmt added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Feb 20, 2024

dkwingsmt and others added 2 commits February 20, 2024 14:32

Add web platform dispatcher test

8731e47

Merge branch 'main' into force-sync-frame

cdeffd9

yjbanov reviewed Feb 20, 2024

View reviewed changes

dkwingsmt added 3 commits February 20, 2024 17:42

Web comments

5d9f48b

Merge remote-tracking branch 'origin/main' into force-sync-frame

1589c59

Fix test

556984f

goderbauer reviewed Feb 21, 2024

View reviewed changes

lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart Outdated Show resolved Hide resolved

Revert timer run change

2b6d3b2

yjbanov approved these changes Feb 22, 2024

View reviewed changes

dkwingsmt merged commit 96d7502 into flutter:main Feb 22, 2024
26 checks passed

dkwingsmt deleted the force-sync-frame branch February 22, 2024 00:49

engine-flutter-autoroll mentioned this pull request Feb 22, 2024

Roll Flutter Engine from bf5c003085fd to 7eeb697687d5 (16 revisions) flutter/flutter#143911

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2024

96d7502a4 Add scheduleWarmUpFrame (flutter/engine#50570)

d74e1c3

dkwingsmt mentioned this pull request Feb 23, 2024

☂️ Multi View for Windows/MacOS flutter/flutter#142845

Open

54 tasks

dkwingsmt mentioned this pull request Feb 23, 2024

Reland 3: Multiview pipeline #49950

Merged

8 tasks

auto-submit bot mentioned this pull request Feb 23, 2024

Reverts "Reland 3: Multiview pipeline (#49950)" #50929

Merged

8 tasks

dkwingsmt mentioned this pull request Apr 4, 2024

Properly render the warmup frames flutter/flutter#142851

Closed

darshankawar mentioned this pull request May 17, 2024

Flutter build with 3.22 for iOS failing: The method 'scheduleWarmUpFrame' isn't defined for the class 'PlatformDispatcher'. flutter/flutter#148414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scheduleWarmUpFrame #50570

Add scheduleWarmUpFrame #50570

dkwingsmt commented Feb 12, 2024 •

edited

Loading

loic-sharma Feb 12, 2024

loic-sharma Feb 12, 2024

dnfield commented Feb 13, 2024

dkwingsmt commented Feb 13, 2024

jonahwilliams commented Feb 13, 2024

jonahwilliams commented Feb 13, 2024

dkwingsmt commented Feb 14, 2024

dkwingsmt commented Feb 15, 2024

jonahwilliams left a comment

yjbanov Feb 20, 2024

dkwingsmt Feb 21, 2024 •

edited

Loading

goderbauer Feb 21, 2024

yjbanov Feb 22, 2024

dkwingsmt Feb 22, 2024

yjbanov Feb 20, 2024

dkwingsmt Feb 21, 2024

goderbauer Feb 21, 2024

yjbanov Feb 22, 2024

yjbanov left a comment

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

		void RequestFrame(bool regenerate_layer_trees = true);

		void ForceSyncFrame();

Add scheduleWarmUpFrame #50570

Add scheduleWarmUpFrame #50570

Conversation

dkwingsmt commented Feb 12, 2024 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Feb 13, 2024

dkwingsmt commented Feb 13, 2024

jonahwilliams commented Feb 13, 2024

jonahwilliams commented Feb 13, 2024

dkwingsmt commented Feb 14, 2024

dkwingsmt commented Feb 15, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkwingsmt Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

dkwingsmt commented Feb 12, 2024 •

edited

Loading

dkwingsmt Feb 21, 2024 •

edited

Loading