SF: Clean up forced refresh and repaint
Remove the misleading repaintEverything flag from CE, as it only affects
debug flashes (and the fullscreen strobe during refresh rate transitions
is of dubious utility). It also causes GPU virtual displays to recompose
despite not being dirty, but forcing repaint can be done by invalidating
geometry instead.
Clean up the Scheduler/SF interface to schedule invalidate, refresh, and
repaint. Fix atomicity of a few debug variables.
Bug: 182939859
Test: Toggle "Show surface updates" in dev settings.
Test: libcompositionengine_test
Change-Id: I176d01cab43e5ab4697b762fe3172b09f4f3d202
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/CompositionRefreshArgs.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/CompositionRefreshArgs.h
index 554e2f4..95d553d 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/CompositionRefreshArgs.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/CompositionRefreshArgs.h
@@ -47,9 +47,6 @@
// All the layers that have queued updates.
Layers layersWithQueuedFrames;
- // If true, forces the entire display to be considered dirty and repainted
- bool repaintEverything{false};
-
// Controls how the color mode is chosen for an output
OutputColorSetting outputColorSetting{OutputColorSetting::kEnhanced};
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
index 28baef8..a33b57d 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
@@ -221,8 +221,7 @@
virtual OutputCompositionState& editState() = 0;
// Gets the dirty region in layer stack space.
- // If repaintEverything is true, this will be the full display bounds.
- virtual Region getDirtyRegion(bool repaintEverything) const = 0;
+ virtual Region getDirtyRegion() const = 0;
// Returns whether the output includes a layer, based on their respective filters.
// See Output::setLayerFilter.
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
index cb9426c..6d49ce6 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
@@ -65,7 +65,7 @@
compositionengine::RenderSurface* getRenderSurface() const override;
void setRenderSurface(std::unique_ptr<compositionengine::RenderSurface>) override;
- Region getDirtyRegion(bool repaintEverything) const override;
+ Region getDirtyRegion() const override;
bool includesLayer(ui::LayerFilter) const override;
bool includesLayer(const sp<LayerFE>&) const override;
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
index 58627da..344b2f9 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
@@ -65,7 +65,7 @@
MOCK_CONST_METHOD0(getState, const OutputCompositionState&());
MOCK_METHOD0(editState, OutputCompositionState&());
- MOCK_CONST_METHOD1(getDirtyRegion, Region(bool));
+ MOCK_METHOD(Region, getDirtyRegion, (), (const));
MOCK_CONST_METHOD1(getOutputLayerForLayer,
compositionengine::OutputLayer*(const sp<compositionengine::LayerFE>&));
diff --git a/services/surfaceflinger/CompositionEngine/src/Display.cpp b/services/surfaceflinger/CompositionEngine/src/Display.cpp
index 2cc5fae..6b9ea87 100644
--- a/services/surfaceflinger/CompositionEngine/src/Display.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Display.cpp
@@ -361,8 +361,7 @@
// 1) It is being handled by hardware composer, which may need this to
// keep its virtual display state machine in sync, or
// 2) There is work to be done (the dirty region isn't empty)
- if (GpuVirtualDisplayId::tryCast(mId) &&
- getDirtyRegion(refreshArgs.repaintEverything).isEmpty()) {
+ if (GpuVirtualDisplayId::tryCast(mId) && getDirtyRegion().isEmpty()) {
ALOGV("Skipping display composition");
return;
}
diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp
index 5e40802..0ff5eca 100644
--- a/services/surfaceflinger/CompositionEngine/src/Output.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp
@@ -368,13 +368,9 @@
mRenderSurface = std::move(surface);
}
-Region Output::getDirtyRegion(bool repaintEverything) const {
+Region Output::getDirtyRegion() const {
const auto& outputState = getState();
- Region dirty(outputState.layerStackSpace.content);
- if (!repaintEverything) {
- dirty.andSelf(outputState.dirtyRegion);
- }
- return dirty;
+ return outputState.dirtyRegion.intersect(outputState.layerStackSpace.content);
}
bool Output::includesLayer(ui::LayerFilter filter) const {
@@ -901,7 +897,7 @@
void Output::beginFrame() {
auto& outputState = editState();
- const bool dirty = !getDirtyRegion(false).isEmpty();
+ const bool dirty = !getDirtyRegion().isEmpty();
const bool empty = getOutputLayerCount() == 0;
const bool wasEmpty = !outputState.lastCompositionHadVisibleLayers;
@@ -953,14 +949,9 @@
}
if (getState().isEnabled) {
- // transform the dirty region into this screen's coordinate space
- const Region dirtyRegion = getDirtyRegion(refreshArgs.repaintEverything);
- if (!dirtyRegion.isEmpty()) {
- base::unique_fd readyFence;
- // redraw the whole screen
+ if (const auto dirtyRegion = getDirtyRegion(); !dirtyRegion.isEmpty()) {
static_cast<void>(composeSurfaces(dirtyRegion, refreshArgs));
-
- mRenderSurface->queueBuffer(std::move(readyFence));
+ mRenderSurface->queueBuffer(base::unique_fd());
}
}
diff --git a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
index 543dde1..f2978f9 100644
--- a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
@@ -879,10 +879,7 @@
mDisplay->editState().layerStackSpace.content = Rect(0, 0, 1, 1);
mDisplay->editState().dirtyRegion = Region::INVALID_REGION;
- CompositionRefreshArgs refreshArgs;
- refreshArgs.repaintEverything = false;
-
- mDisplay->finishFrame(refreshArgs);
+ mDisplay->finishFrame({});
}
TEST_F(DisplayFinishFrameTest, skipsCompositionIfNotDirty) {
@@ -900,10 +897,7 @@
gpuDisplay->editState().layerStackSpace.content = Rect(0, 0, 1, 1);
gpuDisplay->editState().dirtyRegion = Region::INVALID_REGION;
- CompositionRefreshArgs refreshArgs;
- refreshArgs.repaintEverything = false;
-
- gpuDisplay->finishFrame(refreshArgs);
+ gpuDisplay->finishFrame({});
}
TEST_F(DisplayFinishFrameTest, performsCompositionIfDirty) {
@@ -921,31 +915,7 @@
gpuDisplay->editState().layerStackSpace.content = Rect(0, 0, 1, 1);
gpuDisplay->editState().dirtyRegion = Region(Rect(0, 0, 1, 1));
- CompositionRefreshArgs refreshArgs;
- refreshArgs.repaintEverything = false;
-
- gpuDisplay->finishFrame(refreshArgs);
-}
-
-TEST_F(DisplayFinishFrameTest, performsCompositionIfRepaintEverything) {
- auto args = getDisplayCreationArgsForGpuVirtualDisplay();
- std::shared_ptr<impl::Display> gpuDisplay = impl::createDisplay(mCompositionEngine, args);
-
- mock::RenderSurface* renderSurface = new StrictMock<mock::RenderSurface>();
- gpuDisplay->setRenderSurfaceForTest(std::unique_ptr<RenderSurface>(renderSurface));
-
- // We expect a single call to queueBuffer when composition is not skipped.
- EXPECT_CALL(*renderSurface, queueBuffer(_)).Times(1);
-
- gpuDisplay->editState().isEnabled = true;
- gpuDisplay->editState().usesClientComposition = false;
- gpuDisplay->editState().layerStackSpace.content = Rect(0, 0, 1, 1);
- gpuDisplay->editState().dirtyRegion = Region::INVALID_REGION;
-
- CompositionRefreshArgs refreshArgs;
- refreshArgs.repaintEverything = true;
-
- gpuDisplay->finishFrame(refreshArgs);
+ gpuDisplay->finishFrame({});
}
/*
diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
index e7fad59..6c510eb 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
@@ -568,29 +568,13 @@
* Output::getDirtyRegion()
*/
-TEST_F(OutputTest, getDirtyRegionWithRepaintEverythingTrue) {
+TEST_F(OutputTest, getDirtyRegion) {
const Rect viewport{100, 200};
mOutput->editState().layerStackSpace.content = viewport;
mOutput->editState().dirtyRegion.set(50, 300);
- {
- Region result = mOutput->getDirtyRegion(true);
-
- EXPECT_THAT(result, RegionEq(Region(viewport)));
- }
-}
-
-TEST_F(OutputTest, getDirtyRegionWithRepaintEverythingFalse) {
- const Rect viewport{100, 200};
- mOutput->editState().layerStackSpace.content = viewport;
- mOutput->editState().dirtyRegion.set(50, 300);
-
- {
- Region result = mOutput->getDirtyRegion(false);
-
- // The dirtyRegion should be clipped to the display bounds.
- EXPECT_THAT(result, RegionEq(Region(Rect(50, 200))));
- }
+ // The dirty region should be clipped to the display bounds.
+ EXPECT_THAT(mOutput->getDirtyRegion(), RegionEq(Region(Rect(50, 200))));
}
/*
@@ -2522,7 +2506,7 @@
struct OutputPartialMock : public OutputPartialMockBase {
// Sets up the helper functions called by the function under test to use
// mock implementations.
- MOCK_CONST_METHOD1(getDirtyRegion, Region(bool));
+ MOCK_METHOD(Region, getDirtyRegion, (), (const));
};
OutputBeginFrameTest() {
@@ -2534,8 +2518,7 @@
struct IfGetDirtyRegionExpectationState
: public CallOrderStateMachineHelper<TestType, IfGetDirtyRegionExpectationState> {
[[nodiscard]] auto ifGetDirtyRegionReturns(Region dirtyRegion) {
- EXPECT_CALL(getInstance()->mOutput, getDirtyRegion(false))
- .WillOnce(Return(dirtyRegion));
+ EXPECT_CALL(getInstance()->mOutput, getDirtyRegion()).WillOnce(Return(dirtyRegion));
return nextState<AndIfGetOutputLayerCountExpectationState>();
}
};
@@ -2675,7 +2658,7 @@
struct OutputPartialMock : public OutputPartialMockBase {
// Sets up the helper functions called by the function under test to use
// mock implementations.
- MOCK_CONST_METHOD1(getDirtyRegion, Region(bool));
+ MOCK_METHOD(Region, getDirtyRegion, (), (const));
MOCK_METHOD2(composeSurfaces,
std::optional<base::unique_fd>(
const Region&, const compositionengine::CompositionRefreshArgs&));
@@ -2703,7 +2686,6 @@
TEST_F(OutputDevOptRepaintFlashTest, doesNothingIfFlashDelayNotSet) {
mRefreshArgs.devOptFlashDirtyRegionsDelay = {};
- mRefreshArgs.repaintEverything = true;
mOutput.mState.isEnabled = true;
mOutput.devOptRepaintFlash(mRefreshArgs);
@@ -2711,7 +2693,6 @@
TEST_F(OutputDevOptRepaintFlashTest, postsAndPreparesANewFrameIfNotEnabled) {
mRefreshArgs.devOptFlashDirtyRegionsDelay = std::chrono::microseconds(1);
- mRefreshArgs.repaintEverything = true;
mOutput.mState.isEnabled = false;
InSequence seq;
@@ -2721,13 +2702,12 @@
mOutput.devOptRepaintFlash(mRefreshArgs);
}
-TEST_F(OutputDevOptRepaintFlashTest, postsAndPreparesANewFrameIfNotDirty) {
+TEST_F(OutputDevOptRepaintFlashTest, postsAndPreparesANewFrameIfEnabled) {
mRefreshArgs.devOptFlashDirtyRegionsDelay = std::chrono::microseconds(1);
- mRefreshArgs.repaintEverything = true;
mOutput.mState.isEnabled = true;
InSequence seq;
- EXPECT_CALL(mOutput, getDirtyRegion(true)).WillOnce(Return(kEmptyRegion));
+ EXPECT_CALL(mOutput, getDirtyRegion()).WillOnce(Return(kEmptyRegion));
EXPECT_CALL(mOutput, postFramebuffer());
EXPECT_CALL(mOutput, prepareFrame());
@@ -2736,11 +2716,10 @@
TEST_F(OutputDevOptRepaintFlashTest, alsoComposesSurfacesAndQueuesABufferIfDirty) {
mRefreshArgs.devOptFlashDirtyRegionsDelay = std::chrono::microseconds(1);
- mRefreshArgs.repaintEverything = false;
mOutput.mState.isEnabled = true;
InSequence seq;
- EXPECT_CALL(mOutput, getDirtyRegion(false)).WillOnce(Return(kNotEmptyRegion));
+ EXPECT_CALL(mOutput, getDirtyRegion()).WillOnce(Return(kNotEmptyRegion));
EXPECT_CALL(mOutput, composeSurfaces(RegionEq(kNotEmptyRegion), Ref(mRefreshArgs)));
EXPECT_CALL(*mRenderSurface, queueBuffer(_));
EXPECT_CALL(mOutput, postFramebuffer());