Skip to content

Commit bddaa24

Browse files
stagadishstagadish
committed
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
1 parent 3001fd6 commit bddaa24

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
@@ -1165,8 +1165,8 @@ status_t SurfaceFlinger::getStaticDisplayInfo(int64_t displayId, ui::StaticDispl
11651165
}
11661166

11671167
Mutex::Autolock lock(mStateLock);
1168-
const auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(displayId));
1169-
const auto displayOpt = mPhysicalDisplays.get(*id).and_then(getDisplayDeviceAndSnapshot());
1168+
const PhysicalDisplayId id = PhysicalDisplayId::fromValue(static_cast<uint64_t>(displayId));
1169+
const auto displayOpt = mPhysicalDisplays.get(id).and_then(getDisplayDeviceAndSnapshot());
11701170

11711171
if (!displayOpt) {
11721172
return NAME_NOT_FOUND;
@@ -1288,9 +1288,9 @@ status_t SurfaceFlinger::getDynamicDisplayInfoFromId(int64_t physicalDisplayId,
12881288

12891289
Mutex::Autolock lock(mStateLock);
12901290

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

12951295
if (!displayOpt) {
12961296
return NAME_NOT_FOUND;
@@ -6722,8 +6722,9 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
67226722
return getDefaultDisplayDevice()->getDisplayToken().promote();
67236723
}
67246724

6725-
if (const auto id = DisplayId::fromValue<PhysicalDisplayId>(value)) {
6726-
return getPhysicalDisplayToken(*id);
6725+
if (const auto token =
6726+
getPhysicalDisplayToken(PhysicalDisplayId::fromValue(value))) {
6727+
return token;
67276728
}
67286729

67296730
ALOGE("Invalid physical display ID");
@@ -6821,10 +6822,10 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
68216822
case 1040: {
68226823
auto future = mScheduler->schedule([&] {
68236824
n = data.readInt32();
6824-
std::optional<PhysicalDisplayId> inputId = std::nullopt;
6825+
PhysicalDisplayId inputId;
68256826
if (uint64_t inputDisplayId; data.readUint64(&inputDisplayId) == NO_ERROR) {
6826-
inputId = DisplayId::fromValue<PhysicalDisplayId>(inputDisplayId);
6827-
if (!inputId || getPhysicalDisplayToken(*inputId)) {
6827+
inputId = PhysicalDisplayId::fromValue(inputDisplayId);
6828+
if (!getPhysicalDisplayToken(inputId)) {
68286829
ALOGE("No display with id: %" PRIu64, inputDisplayId);
68296830
return NAME_NOT_FOUND;
68306831
}
@@ -6833,7 +6834,7 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
68336834
Mutex::Autolock lock(mStateLock);
68346835
mLayerCachingEnabled = n != 0;
68356836
for (const auto& [_, display] : mDisplays) {
6836-
if (!inputId || *inputId == display->getPhysicalId()) {
6837+
if (inputId == display->getPhysicalId()) {
68376838
display->enableLayerCaching(mLayerCachingEnabled);
68386839
}
68396840
}
@@ -6916,11 +6917,10 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r
69166917
int64_t arg1 = data.readInt64();
69176918
int64_t arg2 = data.readInt64();
69186919
// Enable mirroring for one display
6919-
const auto display1id = DisplayId::fromValue(arg1);
69206920
auto mirrorRoot = SurfaceComposerClient::getDefault()->mirrorDisplay(
6921-
display1id.value());
6922-
auto id2 = DisplayId::fromValue<PhysicalDisplayId>(arg2);
6923-
const auto token2 = getPhysicalDisplayToken(*id2);
6921+
DisplayId::fromValue(arg1));
6922+
const auto token2 =
6923+
getPhysicalDisplayToken(PhysicalDisplayId::fromValue(arg2));
69246924
ui::LayerStack layerStack;
69256925
{
69266926
Mutex::Autolock lock(mStateLock);
@@ -8685,8 +8685,8 @@ binder::Status SurfaceComposerAIDL::getPhysicalDisplayToken(int64_t displayId,
86858685
if (status != OK) {
86868686
return binderStatusFromStatusT(status);
86878687
}
8688-
const auto id = DisplayId::fromValue<PhysicalDisplayId>(static_cast<uint64_t>(displayId));
8689-
*outDisplay = mFlinger->getPhysicalDisplayToken(*id);
8688+
const PhysicalDisplayId id = PhysicalDisplayId::fromValue(static_cast<uint64_t>(displayId));
8689+
*outDisplay = mFlinger->getPhysicalDisplayToken(id);
86908690
return binder::Status::ok();
86918691
}
86928692

‎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)