Merge "SF: Color changes should dirty entire display"

3 files changed, 55 insertions, 26 deletions

diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp
index 6d5147db33..69bb2d979f 100644
--- a/services/surfaceflinger/CompositionEngine/src/Output.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp
@@ -90,13 +90,25 @@ void Output::setLayerStackFilter(uint32_t layerStackId, bool isInternal) {
 
 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 @@ void Output::setColorMode(ui::ColorMode mode, ui::Dataspace dataspace,
     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 8cb6936005..cd2d454938 100644
--- a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
@@ -156,17 +156,17 @@ TEST_F(DisplayTest, setColorModeSetsModeUnlessNoChange) {
     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 @@ TEST_F(DisplayTest, setColorModeDoesNothingForVirtualDisplay) {
     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 cc1211bf1a..a6c6dcf1ad 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
@@ -43,15 +43,21 @@ public:
         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, canInstantiateOutput) {
  */
 
 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, setCompositionEnabledDoesNothingIfAlreadyEnabled) {
 }
 
 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, setProjectionTriviallyWorks) {
  */
 
 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, setBoundsSetsSizeAndDirtiesEntireOutput) {
  */
 
 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 @@ TEST_F(OutputTest, setColorTransformSetsTransform) {
 
     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()));
 }
 
 /* ------------------------------------------------------------------------