Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit ef958122 authored by Lloyd Pique's avatar Lloyd Pique
Browse files

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
parent aa8756d2

services/surfaceflinger/CompositionEngine/src/Output.cpp

+16 −2
Original line number Diff line number Diff line
@@ -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 {

services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp

+6 −6
Original line number Diff line number Diff line
@@ -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);

services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp

+33 −18
Original line number Diff line number Diff line
@@ -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()));

}



/* ------------------------------------------------------------------------