SF: Turn DisplayId::fromValue to an explicit wrapper
Currently, fromValue() would run a tryCast() check before returning
an object, otherwise returning a nullopt. This is now an issue because
tryCast() is reading the bits in the ID value, which is something we are
trying to eliminate.
Remove the tryCast() checks from fromValue(), thus relaxing it and
turning it into a simple wrapper. Since fromValue() can no longer fail,
make it always return the requested type. It is now up to SF to validate
given DisplayIds via its different APIs.
Flag: com.android.graphics.surfaceflinger.flags.stable_edid_ids
Bug: 393193354
Test: Display{Id | Identification}Test && libsurfaceflinger_unittest
Change-Id: I0858567a1769bd94609919bd64bc471f4310ae55
diff --git a/cmds/flatland/Main.cpp b/cmds/flatland/Main.cpp
index 6d14d56..277300d 100644
--- a/cmds/flatland/Main.cpp
+++ b/cmds/flatland/Main.cpp
@@ -772,8 +772,8 @@
break;
case 'i':
- displayId = DisplayId::fromValue<PhysicalDisplayId>(atoll(optarg));
- if (!displayId) {
+ displayId = PhysicalDisplayId::fromValue(atoll(optarg));
+ if (std::find(ids.begin(), ids.end(), displayId) == ids.end()) {
fprintf(stderr, "Invalid display ID: %s.\n", optarg);
exit(4);
}
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 852885b..f964c4d 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -1415,9 +1415,8 @@
ComposerServiceAIDL::getComposerService()->getPhysicalDisplayIds(&displayIds);
if (status.isOk()) {
physicalDisplayIds.reserve(displayIds.size());
- for (auto item : displayIds) {
- auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(item));
- physicalDisplayIds.push_back(*id);
+ for (auto id : displayIds) {
+ physicalDisplayIds.push_back(PhysicalDisplayId::fromValue(static_cast<uint64_t>(id)));
}
}
return physicalDisplayIds;
diff --git a/libs/ui/DisplayIdentification.cpp b/libs/ui/DisplayIdentification.cpp
index b7ef9b3..78e84fc 100644
--- a/libs/ui/DisplayIdentification.cpp
+++ b/libs/ui/DisplayIdentification.cpp
@@ -444,7 +444,7 @@
(edid.hashedBlockZeroSerialNumberOpt.value_or(0) >> 11) ^
(edid.hashedDescriptorBlockSerialNumberOpt.value_or(0) << 23);
- return PhysicalDisplayId::fromEdidHash(id);
+ return PhysicalDisplayId::fromValue(id);
}
} // namespace android
diff --git a/libs/ui/include/ui/DisplayId.h b/libs/ui/include/ui/DisplayId.h
index 13ed754..937e3f1 100644
--- a/libs/ui/include/ui/DisplayId.h
+++ b/libs/ui/include/ui/DisplayId.h
@@ -30,27 +30,16 @@
// Flag indicating that the display is virtual.
static constexpr uint64_t FLAG_VIRTUAL = 1ULL << 63;
- // TODO(b/162612135) Remove default constructor
+ // TODO: b/162612135 - Remove default constructor.
DisplayId() = default;
constexpr DisplayId(const DisplayId&) = default;
DisplayId& operator=(const DisplayId&) = default;
+ static constexpr DisplayId fromValue(uint64_t value) { return DisplayId(value); }
constexpr bool isVirtual() const { return value & FLAG_VIRTUAL; }
uint64_t value;
- // For deserialization.
- static constexpr std::optional<DisplayId> fromValue(uint64_t);
-
- // As above, but also upcast to Id.
- template <typename Id>
- static constexpr std::optional<Id> fromValue(uint64_t value) {
- if (const auto id = Id::tryCast(DisplayId(value))) {
- return id;
- }
- return {};
- }
-
protected:
explicit constexpr DisplayId(uint64_t id) : value(id) {}
};
@@ -74,6 +63,9 @@
// DisplayId of a physical display, such as the internal display or externally connected display.
struct PhysicalDisplayId : DisplayId {
+ // TODO: b/162612135 - Remove default constructor.
+ PhysicalDisplayId() = default;
+
static constexpr ftl::Optional<PhysicalDisplayId> tryCast(DisplayId id) {
if (id.isVirtual()) {
return std::nullopt;
@@ -87,11 +79,6 @@
return PhysicalDisplayId(FLAG_STABLE, port, manufacturerId, modelHash);
}
- // Returns a stable and consistent ID based exclusively on EDID information.
- static constexpr PhysicalDisplayId fromEdidHash(uint64_t hashedEdid) {
- return PhysicalDisplayId(hashedEdid);
- }
-
// Returns an unstable ID. If EDID is available using "fromEdid" is preferred.
static constexpr PhysicalDisplayId fromPort(uint8_t port) {
constexpr uint16_t kManufacturerId = 0;
@@ -99,8 +86,9 @@
return PhysicalDisplayId(0, port, kManufacturerId, kModelHash);
}
- // TODO(b/162612135) Remove default constructor
- PhysicalDisplayId() = default;
+ static constexpr PhysicalDisplayId fromValue(uint64_t value) {
+ return PhysicalDisplayId(value);
+ }
constexpr uint8_t getPort() const { return static_cast<uint8_t>(value); }
@@ -131,8 +119,15 @@
return std::nullopt;
}
+ static constexpr VirtualDisplayId fromValue(uint64_t value) {
+ return VirtualDisplayId(SkipVirtualFlag{}, value);
+ }
+
protected:
+ struct SkipVirtualFlag {};
+ constexpr VirtualDisplayId(SkipVirtualFlag, uint64_t value) : DisplayId(value) {}
explicit constexpr VirtualDisplayId(uint64_t value) : DisplayId(FLAG_VIRTUAL | value) {}
+
explicit constexpr VirtualDisplayId(DisplayId other) : DisplayId(other) {}
};
@@ -146,8 +141,12 @@
return std::nullopt;
}
+ static constexpr HalVirtualDisplayId fromValue(uint64_t value) {
+ return HalVirtualDisplayId(SkipVirtualFlag{}, value);
+ }
+
private:
- explicit constexpr HalVirtualDisplayId(DisplayId other) : VirtualDisplayId(other) {}
+ using VirtualDisplayId::VirtualDisplayId;
};
struct GpuVirtualDisplayId : VirtualDisplayId {
@@ -160,8 +159,12 @@
return std::nullopt;
}
+ static constexpr GpuVirtualDisplayId fromValue(uint64_t value) {
+ return GpuVirtualDisplayId(SkipVirtualFlag{}, value);
+ }
+
private:
- explicit constexpr GpuVirtualDisplayId(DisplayId other) : VirtualDisplayId(other) {}
+ using VirtualDisplayId::VirtualDisplayId;
};
// HalDisplayId is the ID of a display which is managed by HWC.
@@ -177,20 +180,13 @@
return HalDisplayId(id);
}
+ static constexpr HalDisplayId fromValue(uint64_t value) { return HalDisplayId(value); }
+
private:
+ using DisplayId::DisplayId;
explicit constexpr HalDisplayId(DisplayId other) : DisplayId(other) {}
};
-constexpr std::optional<DisplayId> DisplayId::fromValue(uint64_t value) {
- if (const auto id = fromValue<PhysicalDisplayId>(value)) {
- return id;
- }
- if (const auto id = fromValue<VirtualDisplayId>(value)) {
- return id;
- }
- return {};
-}
-
static_assert(sizeof(DisplayId) == sizeof(uint64_t));
static_assert(sizeof(HalDisplayId) == sizeof(uint64_t));
static_assert(sizeof(VirtualDisplayId) == sizeof(uint64_t));
diff --git a/libs/ui/tests/DisplayId_test.cpp b/libs/ui/tests/DisplayId_test.cpp
index 090d2ee..209acba 100644
--- a/libs/ui/tests/DisplayId_test.cpp
+++ b/libs/ui/tests/DisplayId_test.cpp
@@ -33,7 +33,7 @@
EXPECT_TRUE(HalDisplayId::tryCast(id));
EXPECT_EQ(id, DisplayId::fromValue(id.value));
- EXPECT_EQ(id, DisplayId::fromValue<PhysicalDisplayId>(id.value));
+ EXPECT_EQ(id, PhysicalDisplayId::fromValue(id.value));
}
TEST(DisplayIdTest, createPhysicalIdFromPort) {
@@ -47,7 +47,7 @@
EXPECT_TRUE(HalDisplayId::tryCast(id));
EXPECT_EQ(id, DisplayId::fromValue(id.value));
- EXPECT_EQ(id, DisplayId::fromValue<PhysicalDisplayId>(id.value));
+ EXPECT_EQ(id, PhysicalDisplayId::fromValue(id.value));
}
TEST(DisplayIdTest, createGpuVirtualId) {
@@ -59,7 +59,7 @@
EXPECT_FALSE(HalDisplayId::tryCast(id));
EXPECT_EQ(id, DisplayId::fromValue(id.value));
- EXPECT_EQ(id, DisplayId::fromValue<GpuVirtualDisplayId>(id.value));
+ EXPECT_EQ(id, GpuVirtualDisplayId::fromValue(id.value));
}
TEST(DisplayIdTest, createVirtualIdFromGpuVirtualId) {
@@ -83,7 +83,7 @@
EXPECT_TRUE(HalDisplayId::tryCast(id));
EXPECT_EQ(id, DisplayId::fromValue(id.value));
- EXPECT_EQ(id, DisplayId::fromValue<HalVirtualDisplayId>(id.value));
+ EXPECT_EQ(id, HalVirtualDisplayId::fromValue(id.value));
}
TEST(DisplayIdTest, createVirtualIdFromHalVirtualId) {
diff --git a/services/automotive/display/AutomotiveDisplayProxyService.cpp b/services/automotive/display/AutomotiveDisplayProxyService.cpp
index d205231..afa6233 100644
--- a/services/automotive/display/AutomotiveDisplayProxyService.cpp
+++ b/services/automotive/display/AutomotiveDisplayProxyService.cpp
@@ -34,10 +34,8 @@
sp<IBinder> displayToken = nullptr;
sp<SurfaceControl> surfaceControl = nullptr;
if (it == mDisplays.end()) {
- if (const auto displayId = DisplayId::fromValue<PhysicalDisplayId>(id)) {
- displayToken = SurfaceComposerClient::getPhysicalDisplayToken(*displayId);
- }
-
+ displayToken =
+ SurfaceComposerClient::getPhysicalDisplayToken(PhysicalDisplayId::fromValue(id));
if (displayToken == nullptr) {
ALOGE("Given display id, 0x%lX, is invalid.", (unsigned long)id);
return nullptr;
@@ -160,11 +158,8 @@
HwDisplayConfig activeConfig;
HwDisplayState activeState;
- sp<IBinder> displayToken;
- if (const auto displayId = DisplayId::fromValue<PhysicalDisplayId>(id)) {
- displayToken = SurfaceComposerClient::getPhysicalDisplayToken(*displayId);
- }
-
+ sp<IBinder> displayToken =
+ SurfaceComposerClient::getPhysicalDisplayToken(PhysicalDisplayId::fromValue(id));
if (displayToken == nullptr) {
ALOGE("Given display id, 0x%lX, is invalid.", (unsigned long)id);
} else {
@@ -197,4 +192,3 @@
} // namespace automotive
} // namespace frameworks
} // namespace android
-
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp
index 77bf145..6088e25 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -110,8 +110,8 @@
LayerCreationArgs args(mFlinger.get(), sp<Client>::fromExisting(this),
"MirrorRoot-" + std::to_string(displayId), 0 /* flags */,
gui::LayerMetadata());
- std::optional<DisplayId> id = DisplayId::fromValue(static_cast<uint64_t>(displayId));
- status_t status = mFlinger->mirrorDisplay(*id, args, *outResult);
+ const DisplayId id = DisplayId::fromValue(static_cast<uint64_t>(displayId));
+ status_t status = mFlinger->mirrorDisplay(id, args, *outResult);
return binderStatusFromStatusT(status);
}
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index e1c5383..a80ad97 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1165,8 +1165,8 @@
}
Mutex::Autolock lock(mStateLock);
- const auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(displayId));
- const auto displayOpt = mPhysicalDisplays.get(*id).and_then(getDisplayDeviceAndSnapshot());
+ const PhysicalDisplayId id = PhysicalDisplayId::fromValue(static_cast<uint64_t>(displayId));
+ const auto displayOpt = mPhysicalDisplays.get(id).and_then(getDisplayDeviceAndSnapshot());
if (!displayOpt) {
return NAME_NOT_FOUND;
@@ -1288,9 +1288,9 @@
Mutex::Autolock lock(mStateLock);
- const auto id_ =
- DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(physicalDisplayId));
- const auto displayOpt = mPhysicalDisplays.get(*id_).and_then(getDisplayDeviceAndSnapshot());
+ const PhysicalDisplayId id =
+ PhysicalDisplayId::fromValue(static_cast<uint64_t>(physicalDisplayId));
+ const auto displayOpt = mPhysicalDisplays.get(id).and_then(getDisplayDeviceAndSnapshot());
if (!displayOpt) {
return NAME_NOT_FOUND;
@@ -6722,8 +6722,9 @@
return getDefaultDisplayDevice()->getDisplayToken().promote();
}
- if (const auto id = DisplayId::fromValue<PhysicalDisplayId>(value)) {
- return getPhysicalDisplayToken(*id);
+ if (const auto token =
+ getPhysicalDisplayToken(PhysicalDisplayId::fromValue(value))) {
+ return token;
}
ALOGE("Invalid physical display ID");
@@ -6821,10 +6822,10 @@
case 1040: {
auto future = mScheduler->schedule([&] {
n = data.readInt32();
- std::optional<PhysicalDisplayId> inputId = std::nullopt;
+ PhysicalDisplayId inputId;
if (uint64_t inputDisplayId; data.readUint64(&inputDisplayId) == NO_ERROR) {
- inputId = DisplayId::fromValue<PhysicalDisplayId>(inputDisplayId);
- if (!inputId || getPhysicalDisplayToken(*inputId)) {
+ inputId = PhysicalDisplayId::fromValue(inputDisplayId);
+ if (!getPhysicalDisplayToken(inputId)) {
ALOGE("No display with id: %" PRIu64, inputDisplayId);
return NAME_NOT_FOUND;
}
@@ -6833,7 +6834,7 @@
Mutex::Autolock lock(mStateLock);
mLayerCachingEnabled = n != 0;
for (const auto& [_, display] : mDisplays) {
- if (!inputId || *inputId == display->getPhysicalId()) {
+ if (inputId == display->getPhysicalId()) {
display->enableLayerCaching(mLayerCachingEnabled);
}
}
@@ -6916,11 +6917,10 @@
int64_t arg1 = data.readInt64();
int64_t arg2 = data.readInt64();
// Enable mirroring for one display
- const auto display1id = DisplayId::fromValue(arg1);
auto mirrorRoot = SurfaceComposerClient::getDefault()->mirrorDisplay(
- display1id.value());
- auto id2 = DisplayId::fromValue<PhysicalDisplayId>(arg2);
- const auto token2 = getPhysicalDisplayToken(*id2);
+ DisplayId::fromValue(arg1));
+ const auto token2 =
+ getPhysicalDisplayToken(PhysicalDisplayId::fromValue(arg2));
ui::LayerStack layerStack;
{
Mutex::Autolock lock(mStateLock);
@@ -8685,8 +8685,8 @@
if (status != OK) {
return binderStatusFromStatusT(status);
}
- const auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(displayId));
- *outDisplay = mFlinger->getPhysicalDisplayToken(*id);
+ const PhysicalDisplayId id = PhysicalDisplayId::fromValue(static_cast<uint64_t>(displayId));
+ *outDisplay = mFlinger->getPhysicalDisplayToken(id);
return binder::Status::ok();
}
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
index 3fead93..81bfc97 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
@@ -193,7 +193,7 @@
}
};
-template <uint64_t displayId>
+template <VirtualDisplayId::BaseId displayId>
struct DisplayIdGetter<HalVirtualDisplayIdType<displayId>> {
static HalVirtualDisplayId get() { return HalVirtualDisplayId(displayId); }
};
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp
index 2d3ebb4..aadff76 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp
@@ -27,7 +27,8 @@
class CreateDisplayTest : public DisplayTransactionTest {
public:
- void createDisplayWithRequestedRefreshRate(const std::string& name, uint64_t displayId,
+ void createDisplayWithRequestedRefreshRate(const std::string& name,
+ VirtualDisplayId::BaseId baseId,
float pacesetterDisplayRefreshRate,
float requestedRefreshRate,
float expectedAdjustedRefreshRate) {
@@ -49,12 +50,10 @@
EXPECT_EQ(display.requestedRefreshRate, Fps::fromValue(requestedRefreshRate));
EXPECT_EQ(name.c_str(), display.displayName);
- std::optional<VirtualDisplayId> vid =
- DisplayId::fromValue<VirtualDisplayId>(displayId | DisplayId::FLAG_VIRTUAL);
- ASSERT_TRUE(vid.has_value());
-
+ const VirtualDisplayId vid = GpuVirtualDisplayId(baseId);
sp<DisplayDevice> device =
- mFlinger.createVirtualDisplayDevice(displayToken, *vid, requestedRefreshRate);
+ mFlinger.createVirtualDisplayDevice(displayToken, vid, requestedRefreshRate);
+
EXPECT_TRUE(device->isVirtual());
device->adjustRefreshRate(Fps::fromValue(pacesetterDisplayRefreshRate));
// verifying desired value