SF: Color changes should dirty entire display
After doing the refactoring of DisplayDevice into
compositionEngine::Output (etc), it became obvious that color matrix and
color mode changes should really be marking the entire output as dirty
so everything is repainted on it.
This patch makes that change, adding tests to verify the intended
behavior.
Test: atest libsurfaceflinger_unittest libcompositionengine_test
Bug: 121291683
Change-Id: Ia02830eb679271884106f773aa3291f191a49669
diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp
index 6d5147d..69bb2d9 100644
--- a/services/surfaceflinger/CompositionEngine/src/Output.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp
@@ -90,13 +90,25 @@
void Output::setColorTransform(const mat4& transform) {
const bool isIdentity = (transform == mat4());
-
- mState.colorTransform =
+ const auto newColorTransform =
isIdentity ? HAL_COLOR_TRANSFORM_IDENTITY : HAL_COLOR_TRANSFORM_ARBITRARY_MATRIX;
+
+ if (mState.colorTransform == newColorTransform) {
+ return;
+ }
+
+ mState.colorTransform = newColorTransform;
+
+ dirtyEntireOutput();
}
void Output::setColorMode(ui::ColorMode mode, ui::Dataspace dataspace,
ui::RenderIntent renderIntent) {
+ if (mState.colorMode == mode && mState.dataspace == dataspace &&
+ mState.renderIntent == renderIntent) {
+ return;
+ }
+
mState.colorMode = mode;
mState.dataspace = dataspace;
mState.renderIntent = renderIntent;
@@ -106,6 +118,8 @@
ALOGV("Set active color mode: %s (%d), active render intent: %s (%d)",
decodeColorMode(mode).c_str(), mode, decodeRenderIntent(renderIntent).c_str(),
renderIntent);
+
+ dirtyEntireOutput();
}
void Output::dump(std::string& out) const {
diff --git a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
index 8cb6936..cd2d454 100644
--- a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
@@ -156,17 +156,17 @@
EXPECT_EQ(ui::RenderIntent::COLORIMETRIC, mDisplay.getState().renderIntent);
// Otherwise if the values are different, updates happen
- EXPECT_CALL(*renderSurface, setBufferDataspace(ui::Dataspace::SRGB)).Times(1);
+ EXPECT_CALL(*renderSurface, setBufferDataspace(ui::Dataspace::DISPLAY_P3)).Times(1);
EXPECT_CALL(mHwComposer,
- setActiveColorMode(DEFAULT_DISPLAY_ID, ui::ColorMode::BT2100_PQ,
+ setActiveColorMode(DEFAULT_DISPLAY_ID, ui::ColorMode::DISPLAY_P3,
ui::RenderIntent::TONE_MAP_COLORIMETRIC))
.Times(1);
- mDisplay.setColorMode(ui::ColorMode::BT2100_PQ, ui::Dataspace::SRGB,
+ mDisplay.setColorMode(ui::ColorMode::DISPLAY_P3, ui::Dataspace::DISPLAY_P3,
ui::RenderIntent::TONE_MAP_COLORIMETRIC);
- EXPECT_EQ(ui::ColorMode::BT2100_PQ, mDisplay.getState().colorMode);
- EXPECT_EQ(ui::Dataspace::SRGB, mDisplay.getState().dataspace);
+ EXPECT_EQ(ui::ColorMode::DISPLAY_P3, mDisplay.getState().colorMode);
+ EXPECT_EQ(ui::Dataspace::DISPLAY_P3, mDisplay.getState().dataspace);
EXPECT_EQ(ui::RenderIntent::TONE_MAP_COLORIMETRIC, mDisplay.getState().renderIntent);
}
@@ -174,7 +174,7 @@
impl::Display virtualDisplay{mCompositionEngine,
DisplayCreationArgs{false, true, DEFAULT_DISPLAY_ID}};
- virtualDisplay.setColorMode(ui::ColorMode::BT2100_PQ, ui::Dataspace::SRGB,
+ virtualDisplay.setColorMode(ui::ColorMode::DISPLAY_P3, ui::Dataspace::DISPLAY_P3,
ui::RenderIntent::TONE_MAP_COLORIMETRIC);
EXPECT_EQ(ui::ColorMode::NATIVE, virtualDisplay.getState().colorMode);
diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
index cc1211b..a6c6dcf 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
@@ -43,15 +43,21 @@
mOutput.setDisplayColorProfileForTest(
std::unique_ptr<DisplayColorProfile>(mDisplayColorProfile));
mOutput.setRenderSurfaceForTest(std::unique_ptr<RenderSurface>(mRenderSurface));
+
+ mOutput.editState().bounds = kDefaultDisplaySize;
}
~OutputTest() override = default;
+ static const Rect kDefaultDisplaySize;
+
StrictMock<mock::CompositionEngine> mCompositionEngine;
mock::DisplayColorProfile* mDisplayColorProfile = new StrictMock<mock::DisplayColorProfile>();
mock::RenderSurface* mRenderSurface = new StrictMock<mock::RenderSurface>();
impl::Output mOutput{mCompositionEngine};
};
+const Rect OutputTest::kDefaultDisplaySize{100, 200};
+
/* ------------------------------------------------------------------------
* Basic construction
*/
@@ -76,8 +82,6 @@
*/
TEST_F(OutputTest, setCompositionEnabledDoesNothingIfAlreadyEnabled) {
- const Rect displaySize{100, 200};
- mOutput.editState().bounds = displaySize;
mOutput.editState().isEnabled = true;
mOutput.setCompositionEnabled(true);
@@ -87,25 +91,21 @@
}
TEST_F(OutputTest, setCompositionEnabledSetsEnabledAndDirtiesEntireOutput) {
- const Rect displaySize{100, 200};
- mOutput.editState().bounds = displaySize;
mOutput.editState().isEnabled = false;
mOutput.setCompositionEnabled(true);
EXPECT_TRUE(mOutput.getState().isEnabled);
- EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(displaySize)));
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(kDefaultDisplaySize)));
}
TEST_F(OutputTest, setCompositionEnabledSetsDisabledAndDirtiesEntireOutput) {
- const Rect displaySize{100, 200};
- mOutput.editState().bounds = displaySize;
mOutput.editState().isEnabled = true;
mOutput.setCompositionEnabled(false);
EXPECT_FALSE(mOutput.getState().isEnabled);
- EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(displaySize)));
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(kDefaultDisplaySize)));
}
/* ------------------------------------------------------------------------
@@ -135,7 +135,7 @@
*/
TEST_F(OutputTest, setBoundsSetsSizeAndDirtiesEntireOutput) {
- const ui::Size displaySize{100, 200};
+ const ui::Size displaySize{200, 400};
EXPECT_CALL(*mRenderSurface, setDisplaySize(displaySize)).Times(1);
EXPECT_CALL(*mRenderSurface, getSize()).WillOnce(ReturnRef(displaySize));
@@ -152,16 +152,13 @@
*/
TEST_F(OutputTest, setLayerStackFilterSetsFilterAndDirtiesEntireOutput) {
- const Rect displaySize{100, 200};
- mOutput.editState().bounds = displaySize;
-
const uint32_t layerStack = 123u;
mOutput.setLayerStackFilter(layerStack, true);
EXPECT_TRUE(mOutput.getState().layerStackInternal);
EXPECT_EQ(layerStack, mOutput.getState().layerStackId);
- EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(displaySize)));
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(kDefaultDisplaySize)));
}
/* ------------------------------------------------------------------------
@@ -176,27 +173,45 @@
EXPECT_EQ(HAL_COLOR_TRANSFORM_IDENTITY, mOutput.getState().colorTransform);
+ // Since identity is the default, the dirty region should be unchanged (empty)
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region()));
+
// Non-identity matrix sets a non-identity state value
const mat4 nonIdentity = mat4() * 2;
mOutput.setColorTransform(nonIdentity);
EXPECT_EQ(HAL_COLOR_TRANSFORM_ARBITRARY_MATRIX, mOutput.getState().colorTransform);
+
+ // Since this is a state change, the entire output should now be dirty.
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(kDefaultDisplaySize)));
}
/* ------------------------------------------------------------------------
* Output::setColorMode
*/
-TEST_F(OutputTest, setColorModeSetsModeUnlessNoChange) {
- EXPECT_CALL(*mRenderSurface, setBufferDataspace(ui::Dataspace::SRGB)).Times(1);
+TEST_F(OutputTest, setColorModeSetsStateAndDirtiesOutputIfChanged) {
+ EXPECT_CALL(*mRenderSurface, setBufferDataspace(ui::Dataspace::DISPLAY_P3)).Times(1);
- mOutput.setColorMode(ui::ColorMode::BT2100_PQ, ui::Dataspace::SRGB,
+ mOutput.setColorMode(ui::ColorMode::DISPLAY_P3, ui::Dataspace::DISPLAY_P3,
ui::RenderIntent::TONE_MAP_COLORIMETRIC);
- EXPECT_EQ(ui::ColorMode::BT2100_PQ, mOutput.getState().colorMode);
- EXPECT_EQ(ui::Dataspace::SRGB, mOutput.getState().dataspace);
+ EXPECT_EQ(ui::ColorMode::DISPLAY_P3, mOutput.getState().colorMode);
+ EXPECT_EQ(ui::Dataspace::DISPLAY_P3, mOutput.getState().dataspace);
EXPECT_EQ(ui::RenderIntent::TONE_MAP_COLORIMETRIC, mOutput.getState().renderIntent);
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region(kDefaultDisplaySize)));
+}
+
+TEST_F(OutputTest, setColorModeDoesNothingIfNoChange) {
+ mOutput.editState().colorMode = ui::ColorMode::DISPLAY_P3;
+ mOutput.editState().dataspace = ui::Dataspace::DISPLAY_P3;
+ mOutput.editState().renderIntent = ui::RenderIntent::TONE_MAP_COLORIMETRIC;
+
+ mOutput.setColorMode(ui::ColorMode::DISPLAY_P3, ui::Dataspace::DISPLAY_P3,
+ ui::RenderIntent::TONE_MAP_COLORIMETRIC);
+
+ EXPECT_THAT(mOutput.getState().dirtyRegion, RegionEq(Region()));
}
/* ------------------------------------------------------------------------