Make DisplayConfigureRequest own DisplayMode clones.
Prior to this change, DisplayConfigureRequest would indicate a
requested mode via a pointer to a DisplayMode owned by the corresponding
DisplaySnapshot. In order to support the virtual modes feature, it is
no longer valid to assume that a requested mode already exists by the
snapshot. This change replaces DisplayMode pointers with unique_ptrs to
enforce that DisplayConfigureRequests will always own their requested
mode.
When creating a request, modes are cloned as needed (if they already
exist on the snapshot), or created anew in the virtual case. When a
request succeeds, the newly applied mode is matched against existing
modes on the snapshot, or cloned and added to the snapshot in the
virtual case.
Bug: b:321725211
Change-Id: I16ff0a25dc1caac09ae930ca5eb5f4e17c28d039
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5606024
Reviewed-by: Gil Dekel <[email protected]>
Reviewed-by: Xiyuan Xia <[email protected]>
Commit-Queue: Andrew Wolfers <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1329024}
diff --git a/ui/display/manager/configure_displays_task.cc b/ui/display/manager/configure_displays_task.cc
index 1f59923..749d64af 100644
--- a/ui/display/manager/configure_displays_task.cc
+++ b/ui/display/manager/configure_displays_task.cc
@@ -79,8 +79,10 @@
if (request.mode == nullptr)
return;
- if (request.mode == request.display->native_mode())
+ if (request.display->native_mode() &&
+ *request.mode == *request.display->native_mode()) {
return;
+ }
LOG(ERROR) << "A mode other than the preferred mode was requested for the "
"internal display: preferred="
@@ -228,7 +230,7 @@
// After a successful configuration, the DisplaySnapshot associated with a
// request needs to have its state updated to reflect the new configuration.
void UpdateSnapshotAfterConfiguration(const DisplayConfigureRequest& request) {
- request.display->set_current_mode(request.mode);
+ request.display->set_current_mode(request.mode.get());
request.display->set_origin(request.origin);
if (request.display->IsVrrCapable()) {
request.display->set_variable_refresh_rate_state(
@@ -242,7 +244,10 @@
const DisplayMode* mode,
const gfx::Point& origin,
bool enable_vrr)
- : display(display), mode(mode), origin(origin), enable_vrr(enable_vrr) {}
+ : display(display),
+ mode(mode ? mode->Clone() : nullptr),
+ origin(origin),
+ enable_vrr(enable_vrr) {}
DisplayConfigureRequest::DisplayConfigureRequest(DisplaySnapshot* display,
const DisplayMode* mode,
@@ -252,6 +257,15 @@
origin,
/*enable_vrr=*/false) {}
+DisplayConfigureRequest::DisplayConfigureRequest(
+ const DisplayConfigureRequest& other)
+ : DisplayConfigureRequest(other.display,
+ other.mode.get(),
+ other.origin,
+ other.enable_vrr) {}
+
+DisplayConfigureRequest::~DisplayConfigureRequest() = default;
+
ConfigureDisplaysTask::ConfigureDisplaysTask(
NativeDisplayDelegate* delegate,
const std::vector<DisplayConfigureRequest>& requests,
@@ -268,7 +282,15 @@
ConfigureDisplaysTask::RequestToOriginalMode::RequestToOriginalMode(
DisplayConfigureRequest* request,
const DisplayMode* original_mode)
- : request(request), original_mode(original_mode) {}
+ : request(request),
+ original_mode(original_mode ? original_mode->Clone() : nullptr) {}
+
+ConfigureDisplaysTask::RequestToOriginalMode::RequestToOriginalMode(
+ const RequestToOriginalMode& other)
+ : RequestToOriginalMode(other.request, other.original_mode.get()) {}
+
+ConfigureDisplaysTask::RequestToOriginalMode::~RequestToOriginalMode() =
+ default;
ConfigureDisplaysTask::~ConfigureDisplaysTask() {
delegate_->RemoveObserver(this);
@@ -283,7 +305,7 @@
LogIfInvalidRequestForInternalDisplay(request);
config_requests.emplace_back(request.display->display_id(), request.origin,
- request.mode, request.enable_vrr);
+ request.mode.get(), request.enable_vrr);
if (is_first_attempt) {
const std::string uma_name_prefix = GetUmaNamePrefixForRequest(request);
@@ -325,8 +347,10 @@
PartitionRequests();
DCHECK(!pending_display_group_requests_.empty());
// Prep the first group
- for (const auto& pair : pending_display_group_requests_.front())
- pair.request->mode = pair.original_mode;
+ for (const auto& pair : pending_display_group_requests_.front()) {
+ pair.request->mode =
+ pair.original_mode ? pair.original_mode->Clone() : nullptr;
+ }
task_status_ = PARTIAL_SUCCESS;
Run();
return;
@@ -339,7 +363,7 @@
final_requests_status_.emplace_back(&request, true);
config_requests.emplace_back(request.display->display_id(), request.origin,
- request.mode, request.enable_vrr);
+ request.mode.get(), request.enable_vrr);
}
display::ModesetFlags modeset_flags{display::ModesetFlag::kCommitModeset};
@@ -366,8 +390,9 @@
// single) pending group fails. There is no point in disabling displays
// that are already disabled from previous attempts and failed to change
// mode.
- for (const auto& pair : pending_display_group_requests_.front())
- pair.request->mode = nullptr;
+ for (const auto& pair : pending_display_group_requests_.front()) {
+ pair.request->mode.reset();
+ }
task_status_ = ERROR;
}
} else {
@@ -377,7 +402,7 @@
last_successful_config_parameters_.clear();
for (const auto& request : requests_) {
last_successful_config_parameters_.emplace_back(
- request.display->display_id(), request.origin, request.mode,
+ request.display->display_id(), request.origin, request.mode.get(),
request.enable_vrr);
}
}
@@ -392,8 +417,10 @@
pending_display_group_requests_.pop();
if (!pending_display_group_requests_.empty()) {
// Prep the next group
- for (const auto& pair : pending_display_group_requests_.front())
- pair.request->mode = pair.original_mode;
+ for (const auto& pair : pending_display_group_requests_.front()) {
+ pair.request->mode =
+ pair.original_mode ? pair.original_mode->Clone() : nullptr;
+ }
Run();
return;
}
@@ -447,8 +474,8 @@
if (connector_id == requests_[j].display->base_connector_id()) {
// Disable all requests in preparation increment connector retries after
// mapping them to their original request.
- request_group.emplace_back(&requests_[j], requests_[j].mode);
- requests_[j].mode = nullptr;
+ request_group.emplace_back(&requests_[j], requests_[j].mode.get());
+ requests_[j].mode.reset();
}
}
@@ -485,7 +512,7 @@
const DisplayMode* next_mode = FindNextMode(*next_request);
if (next_mode) {
- next_request->mode = next_mode;
+ next_request->mode = next_mode->Clone();
return true;
}
}
