Skip to content

Commit 3f9ee01

Browse files
stagadishAndroid (Google) Code Review
stagadish
authored and
Android (Google) Code Review
committed
Merge "SF: Turn DisplayId::fromValue to an explicit wrapper" into main
2 parents f7d8ed5 + bddaa24 commit 3f9ee01

10 files changed

Lines changed: 66 additions & 78 deletions

File tree

‎cmds/flatland/Main.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -772,8 +772,8 @@ int main(int argc, char** argv) {
772772
break;
773773

774774
case 'i':
775-
displayId = DisplayId::fromValue<PhysicalDisplayId>(atoll(optarg));
776-
if (!displayId) {
775+
displayId = PhysicalDisplayId::fromValue(atoll(optarg));
776+
if (std::find(ids.begin(), ids.end(), displayId) == ids.end()) {
777777
fprintf(stderr, "Invalid display ID: %s.\n", optarg);
778778
exit(4);
779779
}

‎libs/gui/SurfaceComposerClient.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1415,9 +1415,8 @@ std::vector<PhysicalDisplayId> SurfaceComposerClient::getPhysicalDisplayIds() {
14151415
ComposerServiceAIDL::getComposerService()->getPhysicalDisplayIds(&displayIds);
14161416
if (status.isOk()) {
14171417
physicalDisplayIds.reserve(displayIds.size());
1418-
for (auto item : displayIds) {
1419-
auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(item));
1420-
physicalDisplayIds.push_back(*id);
1418+
for (auto id : displayIds) {
1419+
physicalDisplayIds.push_back(PhysicalDisplayId::fromValue(static_cast<uint64_t>(id)));
14211420
}
14221421
}
14231422
return physicalDisplayIds;

‎libs/ui/DisplayIdentification.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ PhysicalDisplayId generateEdidDisplayId(const Edid& edid) {
444444
(edid.hashedBlockZeroSerialNumberOpt.value_or(0) >> 11) ^
445445
(edid.hashedDescriptorBlockSerialNumberOpt.value_or(0) << 23);
446446

447-
return PhysicalDisplayId::fromEdidHash(id);
447+
return PhysicalDisplayId::fromValue(id);
448448
}
449449

450450
} // namespace android

‎libs/ui/include/ui/DisplayId.h‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -30,27 +30,16 @@ struct DisplayId {
3030
// Flag indicating that the display is virtual.
3131
static constexpr uint64_t FLAG_VIRTUAL = 1ULL << 63;
3232

33-
// TODO(b/162612135) Remove default constructor
33+
// TODO: b/162612135 - Remove default constructor.
3434
DisplayId() = default;
3535
constexpr DisplayId(const DisplayId&) = default;
3636
DisplayId& operator=(const DisplayId&) = default;
3737

38+
static constexpr DisplayId fromValue(uint64_t value) { return DisplayId(value); }
3839
constexpr bool isVirtual() const { return value & FLAG_VIRTUAL; }
3940

4041
uint64_t value;
4142

42-
// For deserialization.
43-
static constexpr std::optional<DisplayId> fromValue(uint64_t);
44-
45-
// As above, but also upcast to Id.
46-
template <typename Id>
47-
static constexpr std::optional<Id> fromValue(uint64_t value) {
48-
if (const auto id = Id::tryCast(DisplayId(value))) {
49-
return id;
50-
}
51-
return {};
52-
}
53-
5443
protected:
5544
explicit constexpr DisplayId(uint64_t id) : value(id) {}
5645
};
@@ -74,6 +63,9 @@ inline std::ostream& operator<<(std::ostream& stream, DisplayId displayId) {
7463

7564
// DisplayId of a physical display, such as the internal display or externally connected display.
7665
struct PhysicalDisplayId : DisplayId {
66+
// TODO: b/162612135 - Remove default constructor.
67+
PhysicalDisplayId() = default;
68+
7769
static constexpr ftl::Optional<PhysicalDisplayId> tryCast(DisplayId id) {
7870
if (id.isVirtual()) {
7971
return std::nullopt;
@@ -87,20 +79,16 @@ struct PhysicalDisplayId : DisplayId {
8779
return PhysicalDisplayId(FLAG_STABLE, port, manufacturerId, modelHash);
8880
}
8981

90-
// Returns a stable and consistent ID based exclusively on EDID information.
91-
static constexpr PhysicalDisplayId fromEdidHash(uint64_t hashedEdid) {
92-
return PhysicalDisplayId(hashedEdid);
93-
}
94-
9582
// Returns an unstable ID. If EDID is available using "fromEdid" is preferred.
9683
static constexpr PhysicalDisplayId fromPort(uint8_t port) {
9784
constexpr uint16_t kManufacturerId = 0;
9885
constexpr uint32_t kModelHash = 0;
9986
return PhysicalDisplayId(0, port, kManufacturerId, kModelHash);
10087
}
10188

102-
// TODO(b/162612135) Remove default constructor
103-
PhysicalDisplayId() = default;
89+
static constexpr PhysicalDisplayId fromValue(uint64_t value) {
90+
return PhysicalDisplayId(value);
91+
}
10492

10593
constexpr uint8_t getPort() const { return static_cast<uint8_t>(value); }
10694

@@ -131,8 +119,15 @@ struct VirtualDisplayId : DisplayId {
131119
return std::nullopt;
132120
}
133121

122+
static constexpr VirtualDisplayId fromValue(uint64_t value) {
123+
return VirtualDisplayId(SkipVirtualFlag{}, value);
124+
}
125+
134126
protected:
127+
struct SkipVirtualFlag {};
128+
constexpr VirtualDisplayId(SkipVirtualFlag, uint64_t value) : DisplayId(value) {}
135129
explicit constexpr VirtualDisplayId(uint64_t value) : DisplayId(FLAG_VIRTUAL | value) {}
130+
136131
explicit constexpr VirtualDisplayId(DisplayId other) : DisplayId(other) {}
137132
};
138133

@@ -146,8 +141,12 @@ struct HalVirtualDisplayId : VirtualDisplayId {
146141
return std::nullopt;
147142
}
148143

144+
static constexpr HalVirtualDisplayId fromValue(uint64_t value) {
145+
return HalVirtualDisplayId(SkipVirtualFlag{}, value);
146+
}
147+
149148
private:
150-
explicit constexpr HalVirtualDisplayId(DisplayId other) : VirtualDisplayId(other) {}
149+
using VirtualDisplayId::VirtualDisplayId;
151150
};
152151

153152
struct GpuVirtualDisplayId : VirtualDisplayId {
@@ -160,8 +159,12 @@ struct GpuVirtualDisplayId : VirtualDisplayId {
160159
return std::nullopt;
161160
}
162161

162+
static constexpr GpuVirtualDisplayId fromValue(uint64_t value) {
163+
return GpuVirtualDisplayId(SkipVirtualFlag{}, value);
164+
}
165+
163166
private:
164-
explicit constexpr GpuVirtualDisplayId(DisplayId other) : VirtualDisplayId(other) {}
167+
using VirtualDisplayId::VirtualDisplayId;
165168
};
166169

167170
// HalDisplayId is the ID of a display which is managed by HWC.
@@ -177,20 +180,13 @@ struct HalDisplayId : DisplayId {
177180
return HalDisplayId(id);
178181
}
179182

183+
static constexpr HalDisplayId fromValue(uint64_t value) { return HalDisplayId(value); }
184+
180185
private:
186+
using DisplayId::DisplayId;
181187
explicit constexpr HalDisplayId(DisplayId other) : DisplayId(other) {}
182188
};
183189

184-
constexpr std::optional<DisplayId> DisplayId::fromValue(uint64_t value) {
185-
if (const auto id = fromValue<PhysicalDisplayId>(value)) {
186-
return id;
187-
}
188-
if (const auto id = fromValue<VirtualDisplayId>(value)) {
189-
return id;
190-
}
191-
return {};
192-
}
193-
194190
static_assert(sizeof(DisplayId) == sizeof(uint64_t));
195191
static_assert(sizeof(HalDisplayId) == sizeof(uint64_t));
196192
static_assert(sizeof(VirtualDisplayId) == sizeof(uint64_t));

‎libs/ui/tests/DisplayId_test.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ TEST(DisplayIdTest, createPhysicalIdFromEdid) {
3333
EXPECT_TRUE(HalDisplayId::tryCast(id));
3434

3535
EXPECT_EQ(id, DisplayId::fromValue(id.value));
36-
EXPECT_EQ(id, DisplayId::fromValue<PhysicalDisplayId>(id.value));
36+
EXPECT_EQ(id, PhysicalDisplayId::fromValue(id.value));
3737
}
3838

3939
TEST(DisplayIdTest, createPhysicalIdFromPort) {
@@ -47,7 +47,7 @@ TEST(DisplayIdTest, createPhysicalIdFromPort) {
4747
EXPECT_TRUE(HalDisplayId::tryCast(id));
4848

4949
EXPECT_EQ(id, DisplayId::fromValue(id.value));
50-
EXPECT_EQ(id, DisplayId::fromValue<PhysicalDisplayId>(id.value));
50+
EXPECT_EQ(id, PhysicalDisplayId::fromValue(id.value));
5151
}
5252

5353
TEST(DisplayIdTest, createGpuVirtualId) {
@@ -59,7 +59,7 @@ TEST(DisplayIdTest, createGpuVirtualId) {
5959
EXPECT_FALSE(HalDisplayId::tryCast(id));
6060

6161
EXPECT_EQ(id, DisplayId::fromValue(id.value));
62-
EXPECT_EQ(id, DisplayId::fromValue<GpuVirtualDisplayId>(id.value));
62+
EXPECT_EQ(id, GpuVirtualDisplayId::fromValue(id.value));
6363
}
6464

6565
TEST(DisplayIdTest, createVirtualIdFromGpuVirtualId) {
@@ -83,7 +83,7 @@ TEST(DisplayIdTest, createHalVirtualId) {
8383
EXPECT_TRUE(HalDisplayId::tryCast(id));
8484

8585
EXPECT_EQ(id, DisplayId::fromValue(id.value));
86-
EXPECT_EQ(id, DisplayId::fromValue<HalVirtualDisplayId>(id.value));
86+
EXPECT_EQ(id, HalVirtualDisplayId::fromValue(id.value));
8787
}
8888

8989
TEST(DisplayIdTest, createVirtualIdFromHalVirtualId) {

‎services/automotive/display/AutomotiveDisplayProxyService.cpp‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,8 @@ AutomotiveDisplayProxyService::getIGraphicBufferProducer(uint64_t id) {
3434
sp<IBinder> displayToken = nullptr;
3535
sp<SurfaceControl> surfaceControl = nullptr;
3636
if (it == mDisplays.end()) {
37-
if (const auto displayId = DisplayId::fromValue<PhysicalDisplayId>(id)) {
38-
displayToken = SurfaceComposerClient::getPhysicalDisplayToken(*displayId);
39-
}
40-
37+
displayToken =
38+
SurfaceComposerClient::getPhysicalDisplayToken(PhysicalDisplayId::fromValue(id));
4139
if (displayToken == nullptr) {
4240
ALOGE("Given display id, 0x%lX, is invalid.", (unsigned long)id);
4341
return nullptr;
@@ -160,11 +158,8 @@ Return<void> AutomotiveDisplayProxyService::getDisplayInfo(uint64_t id, getDispl
160158
HwDisplayConfig activeConfig;
161159
HwDisplayState activeState;
162160

163-
sp<IBinder> displayToken;
164-
if (const auto displayId = DisplayId::fromValue<PhysicalDisplayId>(id)) {
165-
displayToken = SurfaceComposerClient::getPhysicalDisplayToken(*displayId);
166-
}
167-
161+
sp<IBinder> displayToken =
162+
SurfaceComposerClient::getPhysicalDisplayToken(PhysicalDisplayId::fromValue(id));
168163
if (displayToken == nullptr) {
169164
ALOGE("Given display id, 0x%lX, is invalid.", (unsigned long)id);
170165
} else {
@@ -197,4 +192,3 @@ Return<void> AutomotiveDisplayProxyService::getDisplayInfo(uint64_t id, getDispl
197192
} // namespace automotive
198193
} // namespace frameworks
199194
} // namespace android
200-

‎services/surfaceflinger/Client.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,8 @@ binder::Status Client::mirrorDisplay(int64_t displayId, gui::CreateSurfaceResult
110110
LayerCreationArgs args(mFlinger.get(), sp<Client>::fromExisting(this),
111111
"MirrorRoot-" + std::to_string(displayId), 0 /* flags */,
112112
gui::LayerMetadata());
113-
std::optional<DisplayId> id = DisplayId::fromValue(static_cast<uint64_t>(displayId));
114-
status_t status = mFlinger->mirrorDisplay(*id, args, *outResult);
113+
const DisplayId id = DisplayId::fromValue(static_cast<uint64_t>(displayId));
114+
status_t status = mFlinger->mirrorDisplay(id, args, *outResult);
115115
return binderStatusFromStatusT(status);
116116
}
117117

‎services/surfaceflinger/SurfaceFlinger.cpp‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1163,8 +1163,8 @@ status_t SurfaceFlinger::getStaticDisplayInfo(int64_t displayId, ui::StaticDispl
11631163
}
11641164

11651165
Mutex::Autolock lock(mStateLock);
1166-
const auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(displayId));
1167-
const auto displayOpt = mPhysicalDisplays.get(*id).and_then(getDisplayDeviceAndSnapshot());
1166+
const PhysicalDisplayId id = PhysicalDisplayId::fromValue(static_cast<uint64_t>(displayId));
1167+
const auto displayOpt = mPhysicalDisplays.get(id).and_then(getDisplayDeviceAndSnapshot());
11681168

11691169
if (!displayOpt) {
11701170
return NAME_NOT_FOUND;
@@ -1286,9 +1286,9 @@ status_t SurfaceFlinger::getDynamicDisplayInfoFromId(int64_t physicalDisplayId,
12861286

12871287
Mutex::Autolock lock(mStateLock);
12881288

1289-
const auto id_ =
1290-
DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(physicalDisplayId));
1291-
const auto displayOpt = mPhysicalDisplays.get(*id_).and_then(getDisplayDeviceAndSnapshot());
1289+
const PhysicalDisplayId id =
1290+
PhysicalDisplayId::fromValue(static_cast<uint64_t>(physicalDisplayId));
1291+
const auto displayOpt = mPhysicalDisplays.get(id).and_then(getDisplayDeviceAndSnapshot());
12921292

12931293
if (!displayOpt) {
12941294
return NAME_NOT_FOUND;
@@ -6731,8 +6731,9 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
67316731
return getDefaultDisplayDevice()->getDisplayToken().promote();
67326732
}
67336733

6734-
if (const auto id = DisplayId::fromValue<PhysicalDisplayId>(value)) {
6735-
return getPhysicalDisplayToken(*id);
6734+
if (const auto token =
6735+
getPhysicalDisplayToken(PhysicalDisplayId::fromValue(value))) {
6736+
return token;
67366737
}
67376738

67386739
ALOGE("Invalid physical display ID");
@@ -6830,10 +6831,10 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
68306831
case 1040: {
68316832
auto future = mScheduler->schedule([&] {
68326833
n = data.readInt32();
6833-
std::optional<PhysicalDisplayId> inputId = std::nullopt;
6834+
PhysicalDisplayId inputId;
68346835
if (uint64_t inputDisplayId; data.readUint64(&inputDisplayId) == NO_ERROR) {
6835-
inputId = DisplayId::fromValue<PhysicalDisplayId>(inputDisplayId);
6836-
if (!inputId || getPhysicalDisplayToken(*inputId)) {
6836+
inputId = PhysicalDisplayId::fromValue(inputDisplayId);
6837+
if (!getPhysicalDisplayToken(inputId)) {
68376838
ALOGE("No display with id: %" PRIu64, inputDisplayId);
68386839
return NAME_NOT_FOUND;
68396840
}
@@ -6842,7 +6843,7 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
68426843
Mutex::Autolock lock(mStateLock);
68436844
mLayerCachingEnabled = n != 0;
68446845
for (const auto& [_, display] : mDisplays) {
6845-
if (!inputId || *inputId == display->getPhysicalId()) {
6846+
if (inputId == display->getPhysicalId()) {
68466847
display->enableLayerCaching(mLayerCachingEnabled);
68476848
}
68486849
}
@@ -6925,11 +6926,10 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
69256926
int64_t arg1 = data.readInt64();
69266927
int64_t arg2 = data.readInt64();
69276928
// Enable mirroring for one display
6928-
const auto display1id = DisplayId::fromValue(arg1);
69296929
auto mirrorRoot = SurfaceComposerClient::getDefault()->mirrorDisplay(
6930-
display1id.value());
6931-
auto id2 = DisplayId::fromValue<PhysicalDisplayId>(arg2);
6932-
const auto token2 = getPhysicalDisplayToken(*id2);
6930+
DisplayId::fromValue(arg1));
6931+
const auto token2 =
6932+
getPhysicalDisplayToken(PhysicalDisplayId::fromValue(arg2));
69336933
ui::LayerStack layerStack;
69346934
{
69356935
Mutex::Autolock lock(mStateLock);
@@ -8714,8 +8714,8 @@ binder::Status SurfaceComposerAIDL::getPhysicalDisplayToken(int64_t displayId,
87148714
if (status != OK) {
87158715
return binderStatusFromStatusT(status);
87168716
}
8717-
const auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(displayId));
8718-
*outDisplay = mFlinger->getPhysicalDisplayToken(*id);
8717+
const PhysicalDisplayId id = PhysicalDisplayId::fromValue(static_cast<uint64_t>(displayId));
8718+
*outDisplay = mFlinger->getPhysicalDisplayToken(id);
87198719
return binder::Status::ok();
87208720
}
87218721

‎services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ struct DisplayIdGetter<PhysicalDisplayIdType<PhysicalDisplay>> {
193193
}
194194
};
195195

196-
template <uint64_t displayId>
196+
template <VirtualDisplayId::BaseId displayId>
197197
struct DisplayIdGetter<HalVirtualDisplayIdType<displayId>> {
198198
static HalVirtualDisplayId get() { return HalVirtualDisplayId(displayId); }
199199
};

‎services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ namespace {
2727

2828
class CreateDisplayTest : public DisplayTransactionTest {
2929
public:
30-
void createDisplayWithRequestedRefreshRate(const std::string& name, uint64_t displayId,
30+
void createDisplayWithRequestedRefreshRate(const std::string& name,
31+
VirtualDisplayId::BaseId baseId,
3132
float pacesetterDisplayRefreshRate,
3233
float requestedRefreshRate,
3334
float expectedAdjustedRefreshRate) {
@@ -49,12 +50,10 @@ class CreateDisplayTest : public DisplayTransactionTest {
4950
EXPECT_EQ(display.requestedRefreshRate, Fps::fromValue(requestedRefreshRate));
5051
EXPECT_EQ(name.c_str(), display.displayName);
5152

52-
std::optional<VirtualDisplayId> vid =
53-
DisplayId::fromValue<VirtualDisplayId>(displayId | DisplayId::FLAG_VIRTUAL);
54-
ASSERT_TRUE(vid.has_value());
55-
53+
const VirtualDisplayId vid = GpuVirtualDisplayId(baseId);
5654
sp<DisplayDevice> device =
57-
mFlinger.createVirtualDisplayDevice(displayToken, *vid, requestedRefreshRate);
55+
mFlinger.createVirtualDisplayDevice(displayToken, vid, requestedRefreshRate);
56+
5857
EXPECT_TRUE(device->isVirtual());
5958
device->adjustRefreshRate(Fps::fromValue(pacesetterDisplayRefreshRate));
6059
// verifying desired value

0 commit comments

Comments
 (0)