SF: Fix a defect in the layer filtering logic
Change 32cbe2 broke the layer filtering logic when moving it over to
CompositionEngine. It would allow certain layers to appear on a virtual
display when they were supposed to be filtered out.
This corrects the logic to be what it was supposed to be, and adds a
unit test that verifies and documents the expected behavior.
Bug: 123248930
Test: atest libsurfaceflinger_unittest libcompositionengine_test
Test: atest android.media.cts.EncodeVirtualDisplayWithCompositionTest
Change-Id: Id2c4b4d32da405c64533924027795620f2d6ee61
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
index 2d00f0c..69b5728 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
@@ -52,11 +52,9 @@
// Sets the bounds to use
virtual void setBounds(const ui::Size&) = 0;
- // Sets the layer stack filter for this output. If singleLayerStack is true,
- // this output displays just the single layer stack specified by
- // singleLayerStackId. Otherwise all layer stacks will be visible on this
- // output.
- virtual void setLayerStackFilter(bool singleLayerStack, uint32_t singleLayerStackId) = 0;
+ // Sets the layer stack filtering settings for this output. See
+ // belongsInOutput for full details.
+ virtual void setLayerStackFilter(uint32_t layerStackId, bool isInternal) = 0;
// Sets the color transform matrix to use
virtual void setColorTransform(const mat4&) = 0;
@@ -96,8 +94,14 @@
// logical (aka layer stack) space.
virtual Region getPhysicalSpaceDirtyRegion(bool repaintEverything) const = 0;
- // Tests whether a given layerStackId belongs in this output
- virtual bool belongsInOutput(uint32_t layerStackId) const = 0;
+ // Tests whether a given layerStackId belongs in this output.
+ // A layer belongs to the output if its layerStackId matches the of the output layerStackId,
+ // unless the layer should display on the primary output only and this is not the primary output
+
+ // A layer belongs to the output if its layerStackId matches. Additionally
+ // if the layer should only show in the internal (primary) display only and
+ // this output allows that.
+ virtual bool belongsInOutput(uint32_t layerStackId, bool internalOnly) const = 0;
protected:
~Output() = default;
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
index 521e1d7..180c40b 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
@@ -38,7 +38,7 @@
void setProjection(const ui::Transform&, int32_t orientation, const Rect& frame,
const Rect& viewport, const Rect& scissor, bool needsFiltering) override;
void setBounds(const ui::Size&) override;
- void setLayerStackFilter(bool singleLayerStack, uint32_t singleLayerStackId) override;
+ void setLayerStackFilter(uint32_t layerStackId, bool isInternal) override;
void setColorTransform(const mat4&) override;
void setColorMode(ui::ColorMode, ui::Dataspace, ui::RenderIntent) override;
@@ -58,7 +58,7 @@
OutputCompositionState& editState() override;
Region getPhysicalSpaceDirtyRegion(bool repaintEverything) const override;
- bool belongsInOutput(uint32_t) const override;
+ bool belongsInOutput(uint32_t, bool) const override;
// Testing
void setDisplayColorProfileForTest(std::unique_ptr<compositionengine::DisplayColorProfile>);
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h
index 96a0e0f..024ed45 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h
@@ -34,12 +34,11 @@
// If false, this output is not considered secure
bool isSecure{false};
- // If true, only layer stacks with a matching layerStack value will be
- // displayed. If false, all layer stacks will be displayed.
- bool singleLayerStack{true};
+ // If true, this output displays layers that are internal-only
+ bool layerStackInternal{false};
// The layer stack to display on this display
- uint32_t singleLayerStackId{~0u};
+ uint32_t layerStackId{~0u};
// The physical space screen bounds
Rect bounds;
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
index 37f7007..445f0bb 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
@@ -35,7 +35,7 @@
MOCK_METHOD6(setProjection,
void(const ui::Transform&, int32_t, const Rect&, const Rect&, const Rect&, bool));
MOCK_METHOD1(setBounds, void(const ui::Size&));
- MOCK_METHOD2(setLayerStackFilter, void(bool, uint32_t));
+ MOCK_METHOD2(setLayerStackFilter, void(uint32_t, bool));
MOCK_METHOD1(setColorTransform, void(const mat4&));
MOCK_METHOD3(setColorMode, void(ui::ColorMode, ui::Dataspace, ui::RenderIntent));
@@ -54,7 +54,7 @@
MOCK_METHOD0(editState, OutputCompositionState&());
MOCK_CONST_METHOD1(getPhysicalSpaceDirtyRegion, Region(bool));
- MOCK_CONST_METHOD1(belongsInOutput, bool(uint32_t));
+ MOCK_CONST_METHOD2(belongsInOutput, bool(uint32_t, bool));
};
} // namespace android::compositionengine::mock
diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp
index dbf77b0..7fb67bd 100644
--- a/services/surfaceflinger/CompositionEngine/src/Output.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp
@@ -75,9 +75,9 @@
dirtyEntireOutput();
}
-void Output::setLayerStackFilter(bool singleLayerStack, uint32_t singleLayerStackId) {
- mState.singleLayerStack = singleLayerStack;
- mState.singleLayerStackId = singleLayerStackId;
+void Output::setLayerStackFilter(uint32_t layerStackId, bool isInternal) {
+ mState.layerStackId = layerStackId;
+ mState.layerStackInternal = isInternal;
dirtyEntireOutput();
}
@@ -175,8 +175,10 @@
return dirty;
}
-bool Output::belongsInOutput(uint32_t layerStackId) const {
- return !mState.singleLayerStack || (layerStackId == mState.singleLayerStackId);
+bool Output::belongsInOutput(uint32_t layerStackId, bool internalOnly) const {
+ // The layerStackId's must match, and also the layer must not be internal
+ // only when not on an internal output.
+ return (layerStackId == mState.layerStackId) && (!internalOnly || mState.layerStackInternal);
}
void Output::dirtyEntireOutput() {
diff --git a/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp b/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp
index 7d8765f..78807ff 100644
--- a/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp
@@ -22,11 +22,9 @@
void OutputCompositionState::dump(std::string& out) const {
dumpVal(out, "isEnabled", isEnabled);
dumpVal(out, "isSecure", isSecure);
- if (singleLayerStack) {
- out.append("layerStack=<any>");
- } else {
- dumpVal(out, "layerStack", singleLayerStackId);
- }
+
+ dumpVal(out, "layerStack", layerStackId);
+ dumpVal(out, "layerStackInternal", layerStackInternal);
out.append("\n ");
diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
index f060cff..fe12825 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
@@ -153,10 +153,10 @@
mOutput.editState().bounds = displaySize;
const uint32_t layerStack = 123u;
- mOutput.setLayerStackFilter(true, layerStack);
+ mOutput.setLayerStackFilter(layerStack, true);
- EXPECT_TRUE(mOutput.getState().singleLayerStack);
- EXPECT_EQ(layerStack, mOutput.getState().singleLayerStackId);
+ EXPECT_TRUE(mOutput.getState().layerStackInternal);
+ EXPECT_EQ(layerStack, mOutput.getState().layerStackId);
EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(displaySize)));
}
@@ -260,5 +260,32 @@
}
}
+/* ------------------------------------------------------------------------
+ * Output::belongsInOutput()
+ */
+
+TEST_F(OutputTest, belongsInOutputFiltersAsExpected) {
+ const uint32_t layerStack1 = 123u;
+ const uint32_t layerStack2 = 456u;
+
+ // If the output accepts layerStack1 and internal-only layers....
+ mOutput.setLayerStackFilter(layerStack1, true);
+
+ // Any layer with layerStack1 belongs to it, internal-only or not.
+ EXPECT_TRUE(mOutput.belongsInOutput(layerStack1, false));
+ EXPECT_TRUE(mOutput.belongsInOutput(layerStack1, true));
+ EXPECT_FALSE(mOutput.belongsInOutput(layerStack2, true));
+ EXPECT_FALSE(mOutput.belongsInOutput(layerStack2, false));
+
+ // If the output accepts layerStack21 but not internal-only layers...
+ mOutput.setLayerStackFilter(layerStack1, false);
+
+ // Only non-internal layers with layerStack1 belong to it.
+ EXPECT_TRUE(mOutput.belongsInOutput(layerStack1, false));
+ EXPECT_FALSE(mOutput.belongsInOutput(layerStack1, true));
+ EXPECT_FALSE(mOutput.belongsInOutput(layerStack2, true));
+ EXPECT_FALSE(mOutput.belongsInOutput(layerStack2, false));
+}
+
} // namespace
} // namespace android::compositionengine
diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp
index 54111de..4a13bfb 100644
--- a/services/surfaceflinger/DisplayDevice.cpp
+++ b/services/surfaceflinger/DisplayDevice.cpp
@@ -166,7 +166,7 @@
// ----------------------------------------------------------------------------
void DisplayDevice::setLayerStack(uint32_t stack) {
- mCompositionDisplay->setLayerStackFilter(!isPrimary(), stack);
+ mCompositionDisplay->setLayerStackFilter(stack, isPrimary());
}
// ----------------------------------------------------------------------------
@@ -330,7 +330,7 @@
}
uint32_t DisplayDevice::getLayerStack() const {
- return mCompositionDisplay->getState().singleLayerStackId;
+ return mCompositionDisplay->getState().layerStackId;
}
const ui::Transform& DisplayDevice::getTransform() const {
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 95a8630..a874a15 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -211,6 +211,7 @@
virtual ~Layer();
void setPrimaryDisplayOnly() { mPrimaryDisplayOnly = true; }
+ bool getPrimaryDisplayOnly() const { return mPrimaryDisplayOnly; }
// ------------------------------------------------------------------------
// Geometry setting functions.
@@ -328,6 +329,9 @@
uint32_t getTransactionFlags(uint32_t flags);
uint32_t setTransactionFlags(uint32_t flags);
+ // Deprecated, please use compositionengine::Output::belongsInOutput()
+ // instead.
+ // TODO(lpique): Move the remaining callers (screencap) to the new function.
bool belongsToDisplay(uint32_t layerStack, bool isPrimaryDisplay) const {
return getLayerStack() == layerStack && (!mPrimaryDisplayOnly || isPrimaryDisplay);
}
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 5df2876..4443df4 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2226,7 +2226,8 @@
mDrawingState.traverseInZOrder([&](Layer* layer) {
bool hwcLayerDestroyed = false;
const auto displayId = displayDevice->getId();
- if (display->belongsInOutput(layer->getLayerStack())) {
+ if (display->belongsInOutput(layer->getLayerStack(),
+ layer->getPrimaryDisplayOnly())) {
Region drawRegion(tr.transform(
layer->visibleNonTransparentRegion));
drawRegion.andSelf(bounds);
@@ -2879,7 +2880,9 @@
// if not, pick the only display it's on.
hintDisplay = nullptr;
for (const auto& [token, display] : mDisplays) {
- if (display->getCompositionDisplay()->belongsInOutput(layer->getLayerStack())) {
+ if (display->getCompositionDisplay()
+ ->belongsInOutput(layer->getLayerStack(),
+ layer->getPrimaryDisplayOnly())) {
if (hintDisplay) {
hintDisplay = nullptr;
break;
@@ -3063,7 +3066,7 @@
const Layer::State& s(layer->getDrawingState());
// only consider the layers on the given layer stack
- if (!display->belongsInOutput(layer->getLayerStack())) {
+ if (!display->belongsInOutput(layer->getLayerStack(), layer->getPrimaryDisplayOnly())) {
return;
}
@@ -3189,7 +3192,7 @@
void SurfaceFlinger::invalidateLayerStack(const sp<const Layer>& layer, const Region& dirty) {
for (const auto& [token, displayDevice] : mDisplays) {
auto display = displayDevice->getCompositionDisplay();
- if (display->belongsInOutput(layer->getLayerStack())) {
+ if (display->belongsInOutput(layer->getLayerStack(), layer->getPrimaryDisplayOnly())) {
display->editState().dirtyRegion.orSelf(dirty);
}
}