Merge "SF: Turn DisplayId::fromValue to an explicit wrapper" into main

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 1dc7b2e702..6384fcc483 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1163,8 +1163,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;
@@ -1286,9 +1286,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;
@@ -6731,8 +6731,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");
@@ -6830,10 +6831,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;
                         }
@@ -6842,7 +6843,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);
                             }
                         }
@@ -6925,11 +6926,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);
@@ -8714,8 +8714,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