Store SurfaceControl reference when creating transactions
The SurfaceControl needs to be stored when adding a new transaction.
This ensures the ref count is incremented so the SC isn't removed before
the transaction is applied.
Change-Id: I27a060e4c221c5dfa565ceb3a916574105fd1175
Fixes: 73448047
Test: DereferenceSurfaceControlTest
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index b7773c4..92a24ad 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -105,12 +105,11 @@
}
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) {
- for (auto const& state : other.mComposerStates) {
- ssize_t index = mComposerStates.indexOf(state);
- if (index < 0) {
- mComposerStates.add(state);
+ for (auto const& kv : other.mComposerStates) {
+ if (mComposerStates.count(kv.first) == 0) {
+ mComposerStates[kv.first] = kv.second;
} else {
- mComposerStates.editItemAt(static_cast<size_t>(index)).state.merge(state.state);
+ mComposerStates[kv.first].state.merge(kv.second.state);
}
}
other.mComposerStates.clear();
@@ -141,7 +140,10 @@
mForceSynchronous |= synchronous;
- composerStates = mComposerStates;
+ for (auto const& kv : mComposerStates){
+ composerStates.add(kv.second);
+ }
+
mComposerStates.clear();
displayStates = mDisplayStates;
@@ -182,18 +184,15 @@
}
layer_state_t* SurfaceComposerClient::Transaction::getLayerState(const sp<SurfaceControl>& sc) {
- ComposerState s;
- s.client = sc->getClient()->mClient;
- s.state.surface = sc->getHandle();
-
- ssize_t index = mComposerStates.indexOf(s);
- if (index < 0) {
+ if (mComposerStates.count(sc) == 0) {
// we don't have it, add an initialized layer_state to our list
- index = mComposerStates.add(s);
+ ComposerState s;
+ s.client = sc->getClient()->mClient;
+ s.state.surface = sc->getHandle();
+ mComposerStates[sc] = s;
}
- ComposerState* const out = mComposerStates.editArray();
- return &(out[index].state);
+ return &(mComposerStates[sc].state);
}
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setPosition(
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index e5156c6..3fe6635 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -19,6 +19,7 @@
#include <stdint.h>
#include <sys/types.h>
+#include <unordered_map>
#include <binder/IBinder.h>
@@ -127,8 +128,14 @@
static status_t injectVSync(nsecs_t when);
+ struct SCHash {
+ std::size_t operator()(const sp<SurfaceControl>& sc) const {
+ return std::hash<SurfaceControl *>{}(sc.get());
+ }
+ };
+
class Transaction {
- SortedVector<ComposerState> mComposerStates;
+ std::unordered_map<sp<SurfaceControl>, ComposerState, SCHash> mComposerStates;
SortedVector<DisplayState > mDisplayStates;
uint32_t mForceSynchronous = 0;
uint32_t mTransactionNestCount = 0;
diff --git a/services/surfaceflinger/tests/SurfaceFlinger_test.filter b/services/surfaceflinger/tests/SurfaceFlinger_test.filter
index be4127c..36424b9 100644
--- a/services/surfaceflinger/tests/SurfaceFlinger_test.filter
+++ b/services/surfaceflinger/tests/SurfaceFlinger_test.filter
@@ -1,5 +1,5 @@
{
"presubmit": {
- "filter": "LayerTransactionTest.*:LayerUpdateTest.*:ChildLayerTest.*:SurfaceFlingerStress.*:CropLatchingTest.*:GeometryLatchingTest.*"
+ "filter": "LayerTransactionTest.*:LayerUpdateTest.*:ChildLayerTest.*:SurfaceFlingerStress.*:CropLatchingTest.*:GeometryLatchingTest.*:ScreenCaptureTest.*:DereferenceSurfaceControlTest.*"
}
}
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index ac8a2ad..92c26af 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -2460,4 +2460,49 @@
ASSERT_EQ(NAME_NOT_FOUND, sf->captureLayers(redLayerHandle, &outBuffer, Rect::EMPTY_RECT, 1.0));
}
+
+class DereferenceSurfaceControlTest : public LayerTransactionTest {
+protected:
+ void SetUp() override {
+ LayerTransactionTest::SetUp();
+ bgLayer = createLayer("BG layer", 20, 20);
+ fillLayerColor(bgLayer, Color::RED);
+ fgLayer = createLayer("FG layer", 20, 20);
+ fillLayerColor(fgLayer, Color::BLUE);
+ Transaction().setLayer(fgLayer, mLayerZBase + 1).apply();
+ {
+ SCOPED_TRACE("before anything");
+ auto shot = screenshot();
+ shot->expectColor(Rect(0, 0, 20, 20), Color::BLUE);
+ }
+ }
+ void TearDown() override {
+ LayerTransactionTest::TearDown();
+ bgLayer = 0;
+ fgLayer = 0;
+ }
+
+ sp<SurfaceControl> bgLayer;
+ sp<SurfaceControl> fgLayer;
+};
+
+TEST_F(DereferenceSurfaceControlTest, LayerNotInTransaction) {
+ fgLayer = nullptr;
+ {
+ SCOPED_TRACE("after setting null");
+ auto shot = screenshot();
+ shot->expectColor(Rect(0, 0, 20, 20), Color::RED);
+ }
+}
+
+TEST_F(DereferenceSurfaceControlTest, LayerInTransaction) {
+ auto transaction = Transaction().show(fgLayer);
+ fgLayer = nullptr;
+ {
+ SCOPED_TRACE("after setting null");
+ auto shot = screenshot();
+ shot->expectColor(Rect(0, 0, 20, 20), Color::BLUE);
+ }
+}
+
} // namespace android