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

10 files changed, 66 insertions, 78 deletions

diff --git a/cmds/flatland/Main.cpp b/cmds/flatland/Main.cpp
index 6d14d568a4..277300d20f 100644
--- a/cmds/flatland/Main.cpp
+++ b/cmds/flatland/Main.cpp
@@ -772,8 +772,8 @@ int main(int argc, char** argv) {
             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 852885be61..f964c4dad2 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -1415,9 +1415,8 @@ std::vector<PhysicalDisplayId> SurfaceComposerClient::getPhysicalDisplayIds() {
             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 b7ef9b3738..78e84fc4dc 100644
--- a/libs/ui/DisplayIdentification.cpp
+++ b/libs/ui/DisplayIdentification.cpp
@@ -444,7 +444,7 @@ PhysicalDisplayId generateEdidDisplayId(const Edid& edid) {
             (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 13ed754534..937e3f1486 100644
--- a/libs/ui/include/ui/DisplayId.h
+++ b/libs/ui/include/ui/DisplayId.h
@@ -30,27 +30,16 @@ struct DisplayId {
     // 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 @@ inline std::ostream& operator<<(std::ostream& stream, DisplayId displayId) {
 
 // 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 @@ struct PhysicalDisplayId : DisplayId {
         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 @@ struct PhysicalDisplayId : DisplayId {
         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 @@ struct VirtualDisplayId : DisplayId {
         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 @@ struct HalVirtualDisplayId : VirtualDisplayId {
         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 @@ struct GpuVirtualDisplayId : VirtualDisplayId {
         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 @@ struct HalDisplayId : DisplayId {
         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 090d2eefc3..209acba672 100644
--- a/libs/ui/tests/DisplayId_test.cpp
+++ b/libs/ui/tests/DisplayId_test.cpp
@@ -33,7 +33,7 @@ TEST(DisplayIdTest, createPhysicalIdFromEdid) {
     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 @@ TEST(DisplayIdTest, createPhysicalIdFromPort) {
     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 @@ TEST(DisplayIdTest, createGpuVirtualId) {
     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 @@ TEST(DisplayIdTest, createHalVirtualId) {
     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 d205231033..afa623352b 100644
--- a/services/automotive/display/AutomotiveDisplayProxyService.cpp
+++ b/services/automotive/display/AutomotiveDisplayProxyService.cpp
@@ -34,10 +34,8 @@ AutomotiveDisplayProxyService::getIGraphicBufferProducer(uint64_t id) {
     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 @@ Return<void> AutomotiveDisplayProxyService::getDisplayInfo(uint64_t id, getDispl
     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 @@ Return<void> AutomotiveDisplayProxyService::getDisplayInfo(uint64_t id, getDispl
 }  // namespace automotive
 }  // namespace frameworks
 }  // namespace android
-
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp
index 77bf1457c3..6088e255df 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -110,8 +110,8 @@ binder::Status Client::mirrorDisplay(int64_t displayId, gui::CreateSurfaceResult
     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 e1c53839e9..a80ad978de 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1165,8 +1165,8 @@ status_t SurfaceFlinger::getStaticDisplayInfo(int64_t displayId, ui::StaticDispl
     }
 
     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 @@ status_t SurfaceFlinger::getDynamicDisplayInfoFromId(int64_t physicalDisplayId,
 
     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 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
                         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 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
             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 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
                         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 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
                         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 @@ binder::Status SurfaceComposerAIDL::getPhysicalDisplayToken(int64_t displayId,
     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 3fead930cc..81bfc977db 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
@@ -193,7 +193,7 @@ struct DisplayIdGetter<PhysicalDisplayIdType<PhysicalDisplay>> {
     }
 };
 
-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 2d3ebb47bd..aadff760fe 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp
@@ -27,7 +27,8 @@ namespace {
 
 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 @@ public:
         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