-
Notifications
You must be signed in to change notification settings - Fork 6k
Embedder API Support for display settings #21355
Embedder API Support for display settings #21355
Conversation
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. |
shell/platform/embedder/embedder.h
Outdated
|
||
FlutterEngineDisplayId display_id; | ||
|
||
/// A double-precision floating-point value representing the actual refresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to describe the meaning of a built-in type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
shell/platform/embedder/embedder.h
Outdated
/// period in seconds. This value may be zero if the device is not running or | ||
/// unavaliable or unknown. | ||
double refresh_rate; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove blank space.
/// connected display or sleeping. | ||
kFlutterEngineDisplaysUpdateTypeStartup, | ||
kFlutterEngineDisplaysUpdateTypeCount, | ||
} FlutterEngineDisplaysUpdateType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for the update type? Is the idea that updates would be deltas rather than just sending the new list?
If so, doesn't that push unnecessary state-tracking information to every embedding, compared to just having the embeddings send a complete display list after any changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was modeling this based on the way platforms seemed to expose the change events, I don't think the embedders need to do a lot of state tracking, the platform apis seem to expose the change events in a similar way:
- On Windows: https://docs.microsoft.com/en-us/uwp/api/windows.devices.display.core.displaymanager.changed?view=winrt-19041
- On macOS: https://developer.apple.com/documentation/coregraphics/cgdisplayreconfigurationcallback?language=objc
I was hoping that would make it easier for the embedders to pipe through the events.
shell/common/display.h
Outdated
/// Some devices do not support multiple displays, in such cases the embedding | ||
/// might not provide a device identifier. This value is used as the device id | ||
/// in such cases. There must only be one screen in such cases. | ||
static constexpr DisplayId kSingleScreenDeviceId = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, shouldn't devices that have multiple displays be prohibited from using this value, so that they can be distinguished?
What if the embedding wants to use a platform-supplied ID that could be zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I wanted to have a reserved value for platforms that only support a single embedder. I can fix the doc here and in the embedder.h
file to indicate the reserved nature of this value.
Do you think 0
is a bad choice for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the concept of it being "unknown" shouldn't be conflated with the id, since the platform might provide zero as a valid ID screen ID (it's pretty likely, actually), and then the embedding would have to do some acrobatics to keep from using zero.
Maybe there should just be another way to find out if this is a single display device, and this just isn't needed? Shouldn't the DisplayManager
have a way to enumerate the displays, and if there's only one, then it's a single-screen device? Ideally, a user of the DisplayManager wouldn't need to have any special code to handle single-display devices verses multi-display devices, they'd just iterate over fewer displays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on our discussion, i've made some changes the ones in this commit specially:
a7f95f3. Hopefully that makes things a little clearer.
shell/common/display.h
Outdated
/// in such cases. There must only be one screen in such cases. | ||
static constexpr DisplayId kSingleScreenDeviceId = 0; | ||
|
||
/// To be used when the display refresh rate is unknown. This is in frames per |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it matters what the units are here if the value is unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove the unit :-)
1e013a8
to
b4f386a
Compare
shell/platform/embedder/embedder.cc
Outdated
for (size_t i = 0; i < display_count; i++) { | ||
if (displays[i].single_display && display_count != 1) { | ||
if (displays[i].single_display && (display_count != 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parens are redundant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std::optional<DisplayId> GetDisplayId() const { return display_id_; } | ||
|
||
private: | ||
std::optional<DisplayId> display_id_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of std::optional
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
shell/common/display.h
Outdated
public: | ||
//------------------------------------------------------------------------------ | ||
/// @brief Construct a new Display object in case where the display id of the | ||
/// display is known. In cases where there are more than one displays, every |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// display is known. In cases where there are more than one displays, every | |
/// display is known. In cases where there is more than one display, every |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.h
Outdated
/// considered active if: | ||
/// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// connected display or sleeping. | |
/// connected display or sleeping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.h
Outdated
std::vector<Display> displays_; | ||
|
||
/// Checks that the provided display configuration is valid. Currently this | ||
/// ensures that all the displays have id in case there are multiple. In case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// ensures that all the displays have id in case there are multiple. In case | |
/// ensures that all the displays have an id in the case where there are multiple displays. In case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.h
Outdated
|
||
/// Checks that the provided display configuration is valid. Currently this | ||
/// ensures that all the displays have id in case there are multiple. In case | ||
/// there is a single display, it is valid for the display to not have an id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// there is a single display, it is valid for the display to not have an id. | |
/// where there is a single display, it is valid for the display to not have an id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.h
Outdated
~DisplayManager(); | ||
|
||
/// Returns the display refresh rate of the main display. In cases where there | ||
/// is only one display connected, it will return that. We do not yer support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// is only one display connected, it will return that. We do not yer support | |
/// is only one display connected, it will return that. We do not yet support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display.h
Outdated
/// @brief Construct a new Display object when there is only a single display. | ||
/// When there are multiple displays, every display must have a display id. | ||
/// | ||
Display(double refresh_rate) : display_id_({}), refresh_rate_(refresh_rate) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this explicit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.cc
Outdated
displays_ = displays; | ||
return; | ||
default: | ||
FML_LOG(ERROR) << "Unknown DisplayUpdateType."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not FML_CHECK
and fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
shell/common/display_manager.h
Outdated
/// Checks that the provided display configuration is valid. Currently this | ||
/// ensures that all the displays have id in case there are multiple. In case | ||
/// there is a single display, it is valid for the display to not have an id. | ||
void CheckDisplayConfiguration(std::vector<Display> displays); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
void DisplayManager::CheckDisplayConfiguration(std::vector<Display> displays) { | ||
FML_CHECK(!displays.empty()); | ||
if (displays.size() > 1) { | ||
for (auto& display : displays) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the displays list changes while you're iterating over it, the iterator will be invalidated. Assuming that's a distinct possibility, since accesses are protected by a lock elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, acquiring lock before this call as well.
//---------------------------------------------------------------------------- | ||
/// @brief Queries the `DisplayManager` for the main display refresh rate. | ||
/// | ||
double GetMainDisplayRefreshRate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.h
Outdated
/// | ||
/// When there are no registered displays, it returns | ||
/// `kUnknownDisplayRefreshRate`. | ||
double GetMainDisplayRefreshRate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
shell/common/display_manager.h
Outdated
|
||
private: | ||
/// Guards `displays_` vector. | ||
std::mutex displays_mutex_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this mutable
to support const
on the getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
- (void)dealloc { | ||
[self invalidate]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we dealloc display_link_
here now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup and done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like calling invalidate does release all strongly held references of the display_link object (from all the run loops). I've also seen this in local testing.
This SO post also indicates the same, I don't think we need to dealloc
it.
shell/platform/embedder/embedder.h
Outdated
/// | ||
FlutterEngineResult FlutterEngineNotifyDisplayUpdate( | ||
FLUTTER_API_SYMBOL(FlutterEngine) engine, | ||
const FlutterEngineDisplaysUpdateType update_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
is unnecessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
6e65ba1
to
4766a62
Compare
Embedders can now notify shell during startup about the various displays and their corresponding settings. Adds a notion of Display update type which can later include chages to displays during runtime such as addition / removal / reconfiguration of displays. We also remove the responsibility of providing the refresh rate from `vsync_waiter` to `DisplayManager`. Rewires existing platform implementations of the said API to use `Shell::OnDisplayUpdate` to notify the display manager of the startup configuration. DisplayManager is also thread-safe to account for rasterizer and UI thread accesses.
4766a62
to
f2ce4ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having trouble getting the test to run locally right now, but I believe this caused the failures seen in recent engine roll attempts on ios_module_test. Can we try to revert and see if the roller gets unblocked? |
* 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)
Embedders can now notify shell during startup about the various displays and their corresponding settings.
Adds a notion of Display update type which can later include changes to displays during runtime such as addition / removal / reconfiguration of displays.
We also remove the responsibility of providing the refresh rate from
vsync_waiter
toDisplayManager
.Rewires existing platform implementations of the said API to use
Shell::OnDisplayUpdate
to notify the display manager of the startup configuration.DisplayManager
is also thread-safe to account for rasterizer and UI thread accesses.