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

Commit 37af25a7 authored by Patrick Williams's avatar Patrick Williams
Browse files

Ensure eLayerSkipScreenshot doesn't hide layers on external displays 

eLayerSkipScreenshot is enforced through separate paths for screen
recording and screen capture.  Prior to this change, for screen
recording, LayerSnapshotBuilder hid mirrored LayerSnapshots when
eLayerSkipScreenshot was set. For screen capture, CompositionEngine
enforced the flag via LayerFilter::toInternalDisplay, filtering out any
layers with eLayerSkipScreenshot when not presenting to an internal
display. The second mechanism caused an issue when using connected
displays. Connected displays aren't internal so eLayerSkipScreenshot
layers were hidden, despite the display not being used for screen
recording or capture.

To fix this issue, the CompositionEngine mechanism is updated to use a
new field, LayerFilter::skipScreenshot. For Outputs,
LayerFilter::skipScreenshot is true only for ScreenCaptureOutputs. For
Layers, LayerFilter::skipScreenshot is true if the layer sets
eLayerSkipScreenshot. This change fixes the issue where
eLayerSkipScreenshot layers are hidden on connected displays but breaks
screen recording scenarios where a layer is being recorded instead of an
entire display because CompositionEngine will no longer filter out
eLayerSkipScreenshot on virtual displays. To fix screen recording, the
LayerSnapshotBuilder mechanism is extended to enforce the flag whenever
layer mirroring is used (as opposed to only enforcing it for display
mirroring).

Bug: 418143858
Flag: com.android.input.flags.connected_displays_cursor
Test: composition engine tests, SF frontend unit tests, manual verification
Change-Id: I311b38682b03896648a21d2291ccc4cc7a239bbe
parent 7b415ac0

services/surfaceflinger/CompositionEngine/Android.bp

+3 −2
Original line number Diff line number Diff line
@@ -162,10 +162,11 @@ cc_test { 
    ],

    static_libs: [

        "libcompositionengine_mocks",

        "libgui_mocks",

        "librenderengine_mocks",

        "libflagtest",

        "libgmock",

        "libgtest",

        "libgui_mocks",

        "librenderengine_mocks",

    ],

    shared_libs: [

        "libbinder_ndk",

services/surfaceflinger/CompositionEngine/src/DumpHelpers.cpp

+1 −0
Original line number Diff line number Diff line
@@ -68,6 +68,7 @@ void dumpVal(std::string& out, const char* name, LayerFilter filter) { 
    out.append("={");

    dumpVal(out, "layerStack", filter.layerStack.id);

    dumpVal(out, "toInternalDisplay", filter.toInternalDisplay);

    dumpVal(out, "skipScreenshot", filter.skipScreenshot);

    out.push_back('}');

}

services/surfaceflinger/CompositionEngine/src/Output.cpp

+9 −2
Original line number Diff line number Diff line
@@ -585,8 +585,15 @@ void Output::ensureOutputLayerIfVisible(sp<compositionengine::LayerFE>& layerFE, 
        return;

    }



    bool computeAboveCoveredExcludingOverlays = coverage.aboveCoveredLayersExcludingOverlays &&

    bool computeAboveCoveredExcludingOverlays = [&]() {

        if (FlagManager::getInstance().connected_displays_cursor()) {

            return coverage.aboveCoveredLayersExcludingOverlays &&

                    !layerFEState->outputFilter.skipScreenshot;

        } else {

            return coverage.aboveCoveredLayersExcludingOverlays &&

                    !layerFEState->outputFilter.toInternalDisplay;

        }

    }();



    /*

     * opaqueRegion: area of a surface that is fully opaque.

services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp

+3 −0
Original line number Diff line number Diff line
@@ -304,6 +304,7 @@ TEST_F(DisplaySetConfigurationTest, configuresPhysicalDisplay) { 
    const auto& filter = mDisplay->getState().layerFilter;

    EXPECT_EQ(ui::UNASSIGNED_LAYER_STACK, filter.layerStack);

    EXPECT_FALSE(filter.toInternalDisplay);

    EXPECT_FALSE(filter.skipScreenshot);

}



TEST_F(DisplaySetConfigurationTest, configuresHalVirtualDisplay) {
@@ -324,6 +325,7 @@ TEST_F(DisplaySetConfigurationTest, configuresHalVirtualDisplay) { 
    const auto& filter = mDisplay->getState().layerFilter;

    EXPECT_EQ(ui::UNASSIGNED_LAYER_STACK, filter.layerStack);

    EXPECT_FALSE(filter.toInternalDisplay);

    EXPECT_FALSE(filter.skipScreenshot);

}



TEST_F(DisplaySetConfigurationTest, configuresGpuVirtualDisplay) {
@@ -344,6 +346,7 @@ TEST_F(DisplaySetConfigurationTest, configuresGpuVirtualDisplay) { 
    const auto& filter = mDisplay->getState().layerFilter;

    EXPECT_EQ(ui::UNASSIGNED_LAYER_STACK, filter.layerStack);

    EXPECT_FALSE(filter.toInternalDisplay);

    EXPECT_FALSE(filter.skipScreenshot);

}



/*

services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp

+94 −9
Original line number Diff line number Diff line
@@ -14,9 +14,15 @@ 
 * limitations under the License.

 */



#include <cstdint>

#include <variant>



#include <android-base/stringprintf.h>

#include <com_android_graphics_libgui_flags.h>

#include <com_android_graphics_surfaceflinger_flags.h>

#include <com_android_input_flags.h>

#include <common/FlagManager.h>

#include <common/test/FlagUtils.h>

#include <compositionengine/LayerFECompositionState.h>

#include <compositionengine/impl/Output.h>

#include <compositionengine/impl/OutputCompositionState.h>
@@ -26,6 +32,7 @@ 
#include <compositionengine/mock/LayerFE.h>

#include <compositionengine/mock/OutputLayer.h>

#include <compositionengine/mock/RenderSurface.h>

#include <flag_macros.h>

#include <ftl/future.h>

#include <gtest/gtest.h>

#include <renderengine/ExternalTexture.h>
@@ -35,13 +42,6 @@ 
#include <ui/Rect.h>

#include <ui/Region.h>



#include <cstdint>

#include <variant>



#include <com_android_graphics_surfaceflinger_flags.h>



#include <common/FlagManager.h>

#include <common/test/FlagUtils.h>

#include "CallOrderStateMachineHelper.h"

#include "RegionMatcher.h"

#include "mock/DisplayHardware/MockHWC2.h"
@@ -50,6 +50,8 @@ 
namespace android::compositionengine {

namespace {



namespace input_flags = com::android::input::flags;



using namespace com::android::graphics::surfaceflinger;



using testing::_;
@@ -472,6 +474,7 @@ TEST_F(OutputTest, setLayerFilterSetsFilterAndDirtiesEntireOutput) { 
    const auto& state = mOutput->getState();

    EXPECT_EQ(kFilter.layerStack, state.layerFilter.layerStack);

    EXPECT_TRUE(state.layerFilter.toInternalDisplay);

    EXPECT_FALSE(state.layerFilter.skipScreenshot);



    EXPECT_THAT(state.dirtyRegion, RegionEq(Region(kDefaultDisplaySize)));

}
@@ -638,7 +641,8 @@ TEST_F(OutputTest, getDirtyRegion) { 
 * Output::includesLayer()

 */



TEST_F(OutputTest, layerFiltering) {

TEST_F_WITH_FLAGS(OutputTest, layerFiltering,

                  REQUIRES_FLAGS_DISABLED(ACONFIG_FLAG(input_flags, connected_displays_cursor))) {

    const ui::LayerStack layerStack1{123u};

    const ui::LayerStack layerStack2{456u};
@@ -665,6 +669,36 @@ TEST_F(OutputTest, layerFiltering) { 
    EXPECT_FALSE(mOutput->includesLayer({layerStack2, false}));

}



TEST_F_WITH_FLAGS(OutputTest, layerFiltering_skipScreenshot,

                  REQUIRES_FLAGS_ENABLED(ACONFIG_FLAG(input_flags, connected_displays_cursor))) {

    const ui::LayerStack layerStack1{123u};

    const ui::LayerStack layerStack2{456u};



    // If the output is associated to layerStack1 and isn't a screenshot...

    mOutput->setLayerFilter({.layerStack = layerStack1, .skipScreenshot = false});



    // It excludes layers with no layer stack, skipScreenshot or not.

    EXPECT_FALSE(mOutput->includesLayer(

            {.layerStack = ui::UNASSIGNED_LAYER_STACK, .skipScreenshot = false}));

    EXPECT_FALSE(mOutput->includesLayer(

            {.layerStack = ui::UNASSIGNED_LAYER_STACK, .skipScreenshot = true}));



    // It includes layers on layerStack1, skipScreenshot or not.

    EXPECT_TRUE(mOutput->includesLayer({.layerStack = layerStack1, .skipScreenshot = false}));

    EXPECT_TRUE(mOutput->includesLayer({.layerStack = layerStack1, .skipScreenshot = true}));

    EXPECT_FALSE(mOutput->includesLayer({.layerStack = layerStack2, .skipScreenshot = true}));

    EXPECT_FALSE(mOutput->includesLayer({.layerStack = layerStack2, .skipScreenshot = false}));



    // If the output is associated to layerStack1 and is a screenshot...

    mOutput->setLayerFilter({.layerStack = layerStack1, .skipScreenshot = true});



    // It includes layers on layerStack1, unless they have skipScreenshot set.

    EXPECT_TRUE(mOutput->includesLayer({.layerStack = layerStack1, .skipScreenshot = false}));

    EXPECT_FALSE(mOutput->includesLayer({.layerStack = layerStack1, .skipScreenshot = true}));

    EXPECT_FALSE(mOutput->includesLayer({.layerStack = layerStack2, .skipScreenshot = true}));

    EXPECT_FALSE(mOutput->includesLayer({.layerStack = layerStack2, .skipScreenshot = false}));

}



TEST_F(OutputTest, layerFilteringWithoutCompositionState) {

    NonInjectedLayer layer;

    sp<LayerFE> layerFE(layer.layerFE);
@@ -674,7 +708,8 @@ TEST_F(OutputTest, layerFilteringWithoutCompositionState) { 
    EXPECT_FALSE(mOutput->includesLayer(layerFE));

}



TEST_F(OutputTest, layerFilteringWithCompositionState) {

TEST_F_WITH_FLAGS(OutputTest, layerFilteringWithCompositionState,

                  REQUIRES_FLAGS_DISABLED(ACONFIG_FLAG(input_flags, connected_displays_cursor))) {

    NonInjectedLayer layer;

    sp<LayerFE> layerFE(layer.layerFE);
@@ -721,6 +756,56 @@ TEST_F(OutputTest, layerFilteringWithCompositionState) { 
    EXPECT_FALSE(mOutput->includesLayer(layerFE));

}



TEST_F_WITH_FLAGS(OutputTest, layerFilteringWithCompositionState_skipScreenshot,

                  REQUIRES_FLAGS_ENABLED(ACONFIG_FLAG(input_flags, connected_displays_cursor))) {

    NonInjectedLayer layer;

    sp<LayerFE> layerFE(layer.layerFE);



    const ui::LayerStack layerStack1{123u};

    const ui::LayerStack layerStack2{456u};



    // If the output is associated to layerStack1 and isn't a screenshot...

    mOutput->setLayerFilter({.layerStack = layerStack1, .skipScreenshot = false});



    // It excludes layers with no layer stack, skipScreenshot or not.

    layer.layerFEState.outputFilter = {.layerStack = ui::UNASSIGNED_LAYER_STACK,

                                       .skipScreenshot = false};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = ui::UNASSIGNED_LAYER_STACK,

                                       .skipScreenshot = true};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));



    // It includes layers on layerStack1, skipScreenshot or not.

    layer.layerFEState.outputFilter = {.layerStack = layerStack1, .skipScreenshot = false};

    EXPECT_TRUE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = layerStack1, .skipScreenshot = true};

    EXPECT_TRUE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = layerStack2, .skipScreenshot = true};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = layerStack2, .skipScreenshot = false};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));



    // If the output is associated to layerStack1 and is a screenshot...

    mOutput->setLayerFilter({.layerStack = layerStack1, .skipScreenshot = true});



    // It includes layers on layerStack1, unless they have skipScreenshot set.

    layer.layerFEState.outputFilter = {.layerStack = layerStack1, .skipScreenshot = false};

    EXPECT_TRUE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = layerStack1, .skipScreenshot = true};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = layerStack2, .skipScreenshot = true};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));



    layer.layerFEState.outputFilter = {.layerStack = layerStack2, .skipScreenshot = false};

    EXPECT_FALSE(mOutput->includesLayer(layerFE));

}



/*

 * Output::getOutputLayerForLayer()

 */