[macOS] Set the display refresh rate #21095

iskakaushik · 2020-09-11T13:23:22Z

This PR depends on #21094 landing first for the embedder API.

flutter-dashboard · 2020-09-11T13:23:24Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

cbracken

stuartmorgan-g · 2020-09-12T13:49:56Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -317,6 +318,29 @@ - (NSOpenGLContext*)resourceContext {
  return _resourceContext;
 }

+- (float)displayRefreshRate {


The style for the macOS embedding is to declare and document all private methods; see the top of this file.

stuartmorgan-g · 2020-09-12T13:51:10Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -317,6 +318,29 @@ - (NSOpenGLContext*)resourceContext {
  return _resourceContext;
 }

+- (float)displayRefreshRate {
+  NSDictionary* description = [[NSScreen mainScreen] deviceDescription];


Why the main screen? What if the Flutter view is on a different screen?

It seems like the proposed embedder API should be reconsidered here to be more flexible for desktop use cases. (E.g., what happens when we support multiple windows, and there are windows on different screens?)

mainScreen is defined as:

The main screen is not necessarily the same screen that contains the menu bar or has its origin at (0, 0). The main screen refers to the screen containing the window that is currently receiving keyboard events. It is the main screen because it is the one with which the user is most likely interacting.

Given that we currently only support 1 window the above is most likely what the devtools wants to display.

In the cases where we support multiple windows, I think we would want the embedder to be able to answer the following questions IMO:

What are all the available displays and their refresh rates?

What displays do each of my windows span?

What configurations/mode are the displays in? (Mirroring for example could limit the refresh rate of the display being mirrored to).

@stuartmorgan are there any other things you think I should consider for the embedder API.

An open question I have is, given that currently devtools and other benchmarking infra only supports configurations with 1 display, I'm wondering if we should treat displays with the most number of windows in them as the main display and use that while we add support for multiple displays and revise this assumption.

Given that we currently only support 1 window

For now; we intend to change that, and should not design new APIs (in a surface where we have a firm policy against any breaking changes) that aren't compatible with that plan.

What displays do each of my windows span?

We'll need that API for multi-window support in general, not just refresh rate, so I think we can assume that will exist.

What configurations/mode are the displays in? (Mirroring for example could limit the refresh rate of the display being mirrored to).

Wouldn't that be reflected in the answer to 1? I.e., wouldn't that just be a singe display with the limed refresh rate?

Wouldn't that be reflected in the answer to 1? I.e., wouldn't that just be a singe display with the limed refresh rate?

I was thinking of this as a static configuration we pass during engine initialization. On reconsideration, I think it makes sense for this to be a callback that the embedder provides to account for dynamic nature of displays. I will update the PR: #21094 to account for these changes.

stuartmorgan-g · 2020-09-12T13:55:41Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

+  double refreshRate = CGDisplayModeGetRefreshRate(displayModeRef);
+
+  // The above can return zero always on some devices.
+  // See: https://github.com/glfw/glfw/issues/137


Why not link to the actual documentation for this, rather than a comment referencing the documentation?

Agreed, will do.

stuartmorgan-g · 2020-09-12T13:56:00Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

+
+  // The above can return zero always on some devices.
+  // See: https://github.com/glfw/glfw/issues/137
+  if (refreshRate == 0.0f) {


Why are you comparing a double to a float literal?

stuartmorgan-g · 2020-09-12T13:56:47Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

+    CVDisplayLinkRelease(displayLinkRef);
+  }
+
+  return static_cast<float>(refreshRate);


Why are we using a float rather than a double?

Related comments on #21094 as well for x-ref.

The API that exposes the display refresh rate: https://github.com/flutter/engine/blob/master/shell/common/vsync_waiter.h#L37 returns a float. That was my motivation for returning a float.

We can change engine internals at any time; once the embedder API is added we are stuck with it. Absent a very strong reason not to, we should use a double here, and if the engine wants to truncate it to a float it can do so.

Fixes: flutter/flutter#49222

iskakaushik · 2020-09-25T21:57:37Z

@gspencergoog , @stuartmorgan , @cbracken I've modified this to use the new APIs. PTAL

gspencergoog

* a1db2b3 Roll Fuchsia Linux SDK from NeYDIjo58... to BFLXvCMVi... (flutter/engine#21403) * faeff0a Roll Dart SDK from eb8e6232da02 to 13b3f2d7b6ea (3 revisions) (flutter/engine#21407) * 7dfcde1 [Fix] Replaces call to deprecated method Name.name. (flutter/engine#21241) * 48ab5d1 Roll Skia from 1748c6a3b8c8 to 3b88c0772e89 (1 revision) (flutter/engine#21410) * aa8d5d4 Avoid sending a 0 DPR to framework (flutter/engine#21389) * 67fdd7e Embedder API Support for display settings (flutter/engine#21355) * 1fc87c0 Roll Skia from 3b88c0772e89 to d7ab45027877 (1 revision) (flutter/engine#21411) * de5f2b4 Revert "Revert "Adds fuchsia node roles to accessibility bridge updates. (#20385)" (#20936)" (flutter/engine#21367) * c9b40c6 Remove ASCII art from mDNS error log (flutter/engine#21397) * fed0531 Roll Fuchsia Mac SDK from xnB_uJM8T... to _e0onA6gY... (flutter/engine#21414) * d877b83 [web] enable ios safari screenshot tests (flutter/engine#21226) * a8f52e5 Roll Skia from d7ab45027877 to aeae3a58e3da (6 revisions) (flutter/engine#21415) * 8275609 Roll Skia from aeae3a58e3da to 7bd60430299f (1 revision) (flutter/engine#21417) * 65c1122 Roll Dart SDK from 13b3f2d7b6ea to 4fb134a228c7 (2 revisions) (flutter/engine#21419) * d735f2c Roll Fuchsia Linux SDK from BFLXvCMVi... to XcAUWQUZm... (flutter/engine#21420) * f1961e5 Roll Skia from 7bd60430299f to 68861e391313 (14 revisions) (flutter/engine#21421) * 08cf725 Fix getNextFrame (flutter/engine#21422) * db99912 Support dragging native platform views (flutter/engine#21396) * df83e8f Roll Skia from 68861e391313 to a05d27b170ee (1 revision) (flutter/engine#21424) * fc7d0fc [web] Respond with null for unimplemented method channels (flutter/engine#21423) * 02b8567 Roll Skia from a05d27b170ee to 5e1545fa00c8 (1 revision) (flutter/engine#21425) * 6e54e68 Roll Dart SDK from 4fb134a228c7 to db7eb2549480 (1 revision) (flutter/engine#21426) * b0cd7d1 Roll Dart SDK from db7eb2549480 to 200e8da5072a (1 revision) (flutter/engine#21427) * 0de5c0c Roll Fuchsia Linux SDK from XcAUWQUZm... to 0nW5DAxcC... (flutter/engine#21430) * 854943d [macOS] Set the display refresh rate (flutter/engine#21095) * 11aecf4 Roll Skia from 5e1545fa00c8 to 766eeb2ac325 (1 revision) (flutter/engine#21431) * b8e2b3f Roll Skia from 766eeb2ac325 to 5648572f4a94 (1 revision) (flutter/engine#21433) * 33d0bbb Roll Fuchsia Mac SDK from _e0onA6gY... to SUSVNJcX5... (flutter/engine#21434) * 51049d2 Roll Skia from 5648572f4a94 to eabce08bb2f2 (1 revision) (flutter/engine#21435) * a491533 Roll Dart SDK from 200e8da5072a to c938793e2d6f (1 revision) (flutter/engine#21437) * 06398b8 Roll Fuchsia Linux SDK from 0nW5DAxcC... to xdxm8rU8b... (flutter/engine#21439) * 4282bbc Roll Dart SDK from c938793e2d6f to fe83360d3a7c (1 revision) (flutter/engine#21440) * eba7b8e Roll Fuchsia Mac SDK from SUSVNJcX5... to v5Ko06GkT... (flutter/engine#21441) * 249bcf7 Roll Fuchsia Linux SDK from xdxm8rU8b... to ej-CkfSra... (flutter/engine#21443) * 4422ede Roll Fuchsia Mac SDK from v5Ko06GkT... to k_lSjZxIH... (flutter/engine#21445) * 35fa4bb Roll Dart SDK from fe83360d3a7c to 44e4f3958019 (1 revision) (flutter/engine#21446) * a9b5b13 Roll Fuchsia Linux SDK from ej-CkfSra... to HNNs4gfuM... (flutter/engine#21447) * 4f7ff21 Roll Skia from eabce08bb2f2 to ad6aeace6eee (2 revisions) (flutter/engine#21448) * f72613d Roll Dart SDK from 44e4f3958019 to e2a4eaba73b8 (1 revision) (flutter/engine#21451) * 2917a65 [ios] Remove unused is_valid_ from IOS Metal Context (flutter/engine#21432) * b591674 Roll Dart SDK from e2a4eaba73b8 to 13deada5b267 (1 revision) (flutter/engine#21453) * a07e0e0 Roll Fuchsia Mac SDK from k_lSjZxIH... to qyoO7f9Sk... (flutter/engine#21454) * cf1fbf2 Apply dpr transform to fuchsia accessibility bridge (flutter/engine#21364) * 9db9a57 Revert "Apply dpr transform to fuchsia accessibility bridge (#21364)" (flutter/engine#21458) * 8d165fa Revert multiple display support for embedder API (flutter/engine#21456)

iskakaushik requested a review from stuartmorgan-g September 11, 2020 13:23

googlebot added the cla: yes label Sep 11, 2020

iskakaushik requested a review from cbracken September 11, 2020 13:23

auto-assign bot requested a review from flar September 11, 2020 13:23

cbracken approved these changes Sep 11, 2020

View reviewed changes

stuartmorgan-g suggested changes Sep 12, 2020

View reviewed changes

iskakaushik mentioned this pull request Sep 14, 2020

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

Closed

[macOS] Set the display refresh rate

03765af

Fixes: flutter/flutter#49222

iskakaushik force-pushed the macos-refresh-rate-1 branch from 6f26063 to 03765af Compare September 25, 2020 21:55

gspencergoog approved these changes Sep 25, 2020

View reviewed changes

iskakaushik merged commit 854943d into flutter:master Sep 26, 2020

engine-flutter-autoroll mentioned this pull request Sep 26, 2020

Roll Engine from 3a73d073c8be to 854943d5c867 (25 revisions) flutter/flutter#66731

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 26, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

443cf05

engine-flutter-autoroll mentioned this pull request Sep 26, 2020

Roll Engine from 3a73d073c8be to 11aecf4dfa7e (26 revisions) flutter/flutter#66734

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 26, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

c695dde

engine-flutter-autoroll mentioned this pull request Sep 26, 2020

Roll Engine from 3a73d073c8be to 33d0bbbc0aa5 (28 revisions) flutter/flutter#66739

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 26, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

366ca4a

engine-flutter-autoroll mentioned this pull request Sep 27, 2020

Roll Engine from 3a73d073c8be to a491533de4b9 (30 revisions) flutter/flutter#66750

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

6b01c72

engine-flutter-autoroll mentioned this pull request Sep 27, 2020

Roll Engine from 3a73d073c8be to 06398b87ec3e (31 revisions) flutter/flutter#66753

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

ba3aa7b

engine-flutter-autoroll mentioned this pull request Sep 27, 2020

Roll Engine from 3a73d073c8be to 4282bbc4bbcd (32 revisions) flutter/flutter#66758

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

6b35285

engine-flutter-autoroll mentioned this pull request Sep 27, 2020

Roll Engine from 3a73d073c8be to eba7b8e60fd3 (33 revisions) flutter/flutter#66759

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

6e32ae0

engine-flutter-autoroll mentioned this pull request Sep 27, 2020

Roll Engine from 3a73d073c8be to 249bcf7a54ce (34 revisions) flutter/flutter#66775

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

1dfb07b

engine-flutter-autoroll mentioned this pull request Sep 27, 2020

Roll Engine from 3a73d073c8be to 35fa4bbdb693 (36 revisions) flutter/flutter#66790

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

c2b2c76

engine-flutter-autoroll mentioned this pull request Sep 28, 2020

Roll Engine from 3a73d073c8be to a9b5b133fa7f (37 revisions) flutter/flutter#66802

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

a100323

engine-flutter-autoroll mentioned this pull request Sep 28, 2020

Roll Engine from 3a73d073c8be to f72613d989f6 (39 revisions) flutter/flutter#66818

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

fbec5bb

engine-flutter-autoroll mentioned this pull request Sep 28, 2020

Roll Engine from 3a73d073c8be to 2917a650110b (40 revisions) flutter/flutter#66831

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

e8714ac

engine-flutter-autoroll mentioned this pull request Sep 28, 2020

Roll Engine from 3a73d073c8be to b5916741dc62 (41 revisions) flutter/flutter#66835

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

bebf9be

engine-flutter-autoroll mentioned this pull request Sep 28, 2020

Roll Engine from 3a73d073c8be to 8d165faca1ce (45 revisions) flutter/flutter#66840

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2020

854943d [macOS] Set the display refresh rate (flutter/engine#21095)

45c97fe

[macOS] Set the display refresh rate #21095

[macOS] Set the display refresh rate #21095

Uh oh!

Conversation

iskakaushik commented Sep 11, 2020

Uh oh!

flutter-dashboard bot commented Sep 11, 2020

Uh oh!

cbracken 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!

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!

cbracken Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 25, 2020

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken Sep 12, 2020 •

edited

Loading