[embedder] Project Arg to specify the display refresh rate #21094

iskakaushik · 2020-09-11T12:51:32Z

This is then wired to the vsync waiter to provide the advisory display refresh rate. embedder_unittests have been amended to test this value both when set and not set.

cbracken · 2020-09-11T20:08:53Z

shell/platform/embedder/embedder.h

+  ///
+  /// This information is purely advisory and is not used by any component. It
+  /// is only used by the tooling to visualize frame performance.
+  float display_refresh_rate;


Not that we're likely to make full use of double-precision, but any particular reason for going with float rather than double here? I'd imagine from a struct-packing perspective, other fields are likely to be 64-bit aligned anyway.

cbracken · 2020-09-11T20:12:04Z

shell/common/vsync_waiter_fallback.h

 public:
  VsyncWaiterFallback(TaskRunners task_runners);

+  VsyncWaiterFallback(TaskRunners task_runners, float display_refresh_rate);


Update the class doc comment to cover this ctor.

liyuqian

Mostly looks good. I'm curious where this will be used. Desktop embedder maybe?

liyuqian · 2020-09-11T20:43:45Z

shell/platform/embedder/tests/embedder_unittests.cc

+  auto vsync_waiter = shell.GetPlatformView()->CreateVSyncWaiter();
+
+  const float embedder_refresh_rate = vsync_waiter->GetDisplayRefreshRate();
+  ASSERT_FLOAT_EQ(refresh_rate, embedder_refresh_rate);


Nit: is it better to be ASSERT_FLOAT_EQ(embedder_refresh_rate, refresh_rate); as ASSERT_FLOAT_EQ(actual, expected)?

good call, done

liyuqian · 2020-09-11T20:46:14Z

shell/platform/embedder/tests/embedder_unittests.cc

+
+  const float embedder_refresh_rate = vsync_waiter->GetDisplayRefreshRate();
+  ASSERT_FLOAT_EQ(flutter::VsyncWaiter::kUnknownRefreshRateFPS,
+                  embedder_refresh_rate);


iskakaushik · 2020-09-14T14:29:09Z

This PR was made for the needs of http://github.com/flutter/engine/pull/21095. The API likely needs to be revised based on the outcome of that PR. Stashing this PR until the comments there are resolved.

iskakaushik · 2020-09-15T00:52:33Z

shell/platform/embedder/embedder.cc

+          platform_message_response_callback,             //
+          vsync_callback,                                 //
+          compute_platform_resolved_locale_callback,      //
+          static_cast<float>(main_display_refresh_rate),  //


Down casting here as this is an internal api and we can change it later if we decide to use the full double precision.

nit: update this to double

iskakaushik · 2020-09-15T00:54:18Z

shell/platform/embedder/platform_view_embedder.h

    VsyncWaiterEmbedder::VsyncCallback vsync_callback;  // optional
    ComputePlatformResolvedLocaleCallback
        compute_platform_resolved_locale_callback;
+    float display_refresh_rate;


I decided not to plumb the callback all the way through to vsync waiter as currently GetDisplayRefreshRate is const and none of the platforms currently need to dynamically return the refresh rate of the display.

Though I believe we will need that at some point in the future.

cbracken · 2020-09-18T18:03:36Z

shell/platform/embedder/embedder.cc

+    }
+
+    display_settings.displays =
+        new FlutterDisplay[display_settings.display_count];


Could we allocate this as a std::vector then hand the callback displays.data() instead to avoid dealing with new/delete?

cbracken · 2020-09-18T18:17:38Z

shell/platform/embedder/embedder.cc

+    // TODO(iskakaushik): Add support for multiple displays.
+    main_display_refresh_rate = display_settings.displays[0].refresh_rate;
+
+    delete display_settings.displays;


nit: this should be delete[] display_settings.displays; if for some reason using a vector isn't workable.

Used vector as recommended :-)

cbracken · 2020-09-18T18:18:26Z

shell/platform/embedder/embedder.cc

+          platform_message_response_callback,             //
+          vsync_callback,                                 //
+          compute_platform_resolved_locale_callback,      //
+          static_cast<float>(main_display_refresh_rate),  //


nit: update this to double

stuartmorgan-g · 2020-09-21T15:48:30Z

shell/platform/embedder/embedder.h


+/// Display refers to a graphics hardware system consisting of a framebuffer,
+/// typically a monitor or a screen. This ID is unique per display and is
+/// stable until the Flutter application restarts.


How would an embedding enforce this? NSScreen doesn't have an ID, for instance, AFAICT.

This was the display ID I was planning on using: https://developer.apple.com/documentation/coregraphics/cgdirectdisplayid?language=objc.

stuartmorgan-g · 2020-09-21T15:50:44Z

shell/platform/embedder/embedder.h

+  /// unavaliable or unknown.
+  double refresh_rate;
+
+} FlutterDisplay;


We should use FlutterEngine as the prefix for new types; I've already had to add a macro to solve one collision problem with macOS, and I'd rather we not introduce more potential collision points.

stuartmorgan-g · 2020-09-21T15:50:50Z

shell/platform/embedder/embedder.h

+  /// active displays.
+  FlutterDisplay* displays;
+
+} FlutterDisplaySettings;


Same note about prefix.

stuartmorgan-g · 2020-09-21T15:51:33Z

shell/platform/embedder/embedder.h

+  /// This size of this struct. Must be sizeof(FlutterDisplaySettings).
+  size_t struct_size;
+
+  /// Contains the actual number of displays returned in the displays array.


s/actual number/number/

s/returned// (since this isn't part of a function, so nothing is being returned)

stuartmorgan-g · 2020-09-21T15:52:19Z

shell/platform/embedder/embedder.h

+
+  /// A pointer to storage provided by the caller for an array of
+  /// FlutterDisplays. On return, the array contains a list of active displays.
+  /// If you pass NULL, on return the display_count contains the total number of


Again, this seems to be describing a function, but it's the comment for a struct element.

Yup, agreed, I will go over the docs and make sure they align with structs and functions.

stuartmorgan-g · 2020-09-21T15:52:38Z

shell/platform/embedder/embedder.h

+
+} FlutterDisplaySettings;
+
+typedef void (*FlutterDisplaySettingsCallback)(


Please document.

stuartmorgan-g · 2020-09-21T15:56:22Z

shell/platform/embedder/embedder.h

+  ///    1. The frame buffer hardware is connected.
+  ///    2. The display is drawable, e.g. it isn't being mirrored from another
+  ///    connected display or sleeping.
+  FlutterDisplaySettingsCallback display_settings_callback;


I'm confused as to how this API would work; how would the engine know when the display list has changed and it needs to call this again? Since the embedder, not the engine, knows when the display list changes, I would expect a method called by the embedder to push information into the engine.

Even if for some reason that's not necessary now, it certainly will be when we're actually exposing display information in Dart, and we shouldn't have two different ways of providing the same information. /cc @gspencergoog

Good point, currently this change isn't accounting for cases where the display configuration changes while the app is running. I'm thinking of modifying the API to notify the engine on display reconfiguration, with 4 modes:

Startup (when the app is launched)

Adding new displays

Removing existing displays

Reconfigure an existing display (modify any display settings)

We might be able to use CGDisplayReconfigurationCallBack on macOS.

WDYT?

Yes, I think we will need those events, and they will have to come from the embedder and notify the engine.

One other thing to note is that some platforms won't be able to provide all of the information. For instance, on UWP and web it's not possible to find out what screens are connected, or their resolutions, if the current view isn't on that screen. And it doesn't provide the information for more than one screen if you span screens.

The screen configuration doesn't even have to change, though. You can just drag the window to another screen (or even just adjust it's position so more than half of it is on another screen on some platforms) and that will change these values for that app.

One other thing to note is that some platforms won't be able to provide all of the information. For instance, on UWP and web it's not possible to find out what screens are connected, or their resolutions, if the current view isn't on that screen. And it doesn't provide the information for more than one screen if you span screens.

I will document that this is best effort, with the only guarantee that embedder MUST provide at least one display setting which will correspond to the main screen (where the app window is active).

The screen configuration doesn't even have to change, though. You can just drag the window to another screen (or even just adjust it's position so more than half of it is on another screen on some platforms) and that will change these values for that app.

I am envisioning that this will result in 2 events being invoked by the embedder:

Add Display: display-2

Remove Display: display-1

Though there is some additional checks that are needed to be done by the embedders to ensure that the engine is aware of at least one display at any given time (i.e, no remove before add), I think it's ok to have this.

I will document that this is best effort, with the only guarantee that embedder MUST provide at least one display setting which will correspond to the main screen (where the app window is active).

Well, but when we have multiple windows, there will be multiple display settings that apply (one or more for each window). Not that you need to solve multi-window here, but maybe the requirement should be that it provide screen settings corresponding to each window displayed.

The api currently supports mutliple displays, but the implementation only wires in the first display we get as the main display refresh rate.

iskakaushik · 2020-09-23T17:00:15Z

This is superseded by #21355.

iskakaushik requested review from cbracken and liyuqian September 11, 2020 12:51

googlebot added the cla: yes label Sep 11, 2020

auto-assign bot requested a review from gw280 September 11, 2020 12:51

iskakaushik mentioned this pull request Sep 11, 2020

[macOS] Set the display refresh rate #21095

Merged

cbracken reviewed Sep 11, 2020

View reviewed changes

liyuqian reviewed Sep 11, 2020

View reviewed changes

iskakaushik force-pushed the refresh-rate branch from b0ff8d4 to 4439679 Compare September 15, 2020 00:51

iskakaushik commented Sep 15, 2020

View reviewed changes

iskakaushik requested a review from stuartmorgan-g September 15, 2020 00:54

cbracken reviewed Sep 18, 2020

View reviewed changes

stuartmorgan-g suggested changes Sep 21, 2020

View reviewed changes

iskakaushik added 2 commits September 21, 2020 08:59

[embedder] Add support for passing display refresh rate

1d3e7fa

The api currently supports mutliple displays, but the implementation only wires in the first display we get as the main display refresh rate.

Fix CR comments

eadc005

iskakaushik force-pushed the refresh-rate branch from 5ae1aed to eadc005 Compare September 21, 2020 16:23

fix unittest

627dde9

iskakaushik closed this Sep 23, 2020


		} FlutterDisplaySettings;

		typedef void (*FlutterDisplaySettingsCallback)(

[embedder] Project Arg to specify the display refresh rate #21094

[embedder] Project Arg to specify the display refresh rate #21094

Uh oh!

Conversation

iskakaushik commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone