From efc42e2badd2be4e3404cc16619dd3d85d37a7e7 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 3 Dec 2019 17:36:12 -0800 Subject: Fix SurfaceControl#captureLayers when the layer is boundless - Return an error if the client tries to screenshot a boundless layer without specifying a crop. Otherwise the client will get a screenshot of 0x0. - Use the crop in addition to the buffer size when determining the bounds of the captured layer. This will enable us to capture container layers and color layers that have a crop specified. Fixes: 141326137 Test: atest SurfaceFlinger_test Test: go/wm-smoke Change-Id: Ibba4c01ad2d6739caee0d85b8d9c2d236fbf0ce0 --- services/surfaceflinger/Layer.h | 15 +++--- services/surfaceflinger/SurfaceFlinger.cpp | 11 ++++- services/surfaceflinger/tests/Credentials_test.cpp | 2 +- services/surfaceflinger/tests/LayerUpdate_test.cpp | 55 ++++++++++++++++++++++ .../surfaceflinger/tests/utils/TransactionUtils.h | 5 ++ 5 files changed, 78 insertions(+), 10 deletions(-) (limited to 'services') diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 7abcd0f131..adf3075d08 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -699,6 +699,14 @@ public: Region debugGetVisibleRegionOnDefaultDisplay() const; + /** + * Returns the cropped buffer size or the layer crop if the layer has no buffer. Return + * INVALID_RECT if the layer has no buffer and no crop. + * A layer with an invalid buffer size and no crop is considered to be boundless. The layer + * bounds are constrained by its parent bounds. + */ + Rect getCroppedBufferSize(const Layer::State& s) const; + protected: // constant sp mFlinger; @@ -907,13 +915,6 @@ private: const LayerVector::Visitor& visitor); LayerVector makeChildrenTraversalList(LayerVector::StateSet stateSet, const std::vector& layersInTree); - /** - * Returns the cropped buffer size or the layer crop if the layer has no buffer. Return - * INVALID_RECT if the layer has no buffer and no crop. - * A layer with an invalid buffer size and no crop is considered to be boundless. The layer - * bounds are constrained by its parent bounds. - */ - Rect getCroppedBufferSize(const Layer::State& s) const; // Cached properties computed from drawing state // Effective transform taking into account parent transforms and any parent scaling. diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index c555f7985f..8b04b657cf 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5067,14 +5067,21 @@ status_t SurfaceFlinger::captureLayers( return PERMISSION_DENIED; } + Rect parentSourceBounds = parent->getCroppedBufferSize(parent->getCurrentState()); if (sourceCrop.width() <= 0) { crop.left = 0; - crop.right = parent->getBufferSize(parent->getCurrentState()).getWidth(); + crop.right = parentSourceBounds.getWidth(); } if (sourceCrop.height() <= 0) { crop.top = 0; - crop.bottom = parent->getBufferSize(parent->getCurrentState()).getHeight(); + crop.bottom = parentSourceBounds.getHeight(); + } + + if (crop.isEmpty() || frameScale <= 0.0f) { + // Error out if the layer has no source bounds (i.e. they are boundless) and a source + // crop was not specified, or an invalid frame scale was provided. + return BAD_VALUE; } reqWidth = crop.width() * frameScale; reqHeight = crop.height() * frameScale; diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp index b667a74db0..b1bb7fdef9 100644 --- a/services/surfaceflinger/tests/Credentials_test.cpp +++ b/services/surfaceflinger/tests/Credentials_test.cpp @@ -274,7 +274,7 @@ TEST_F(CredentialsTest, CaptureLayersTest) { sp outBuffer; return ScreenshotClient::captureLayers(mBGSurfaceControl->getHandle(), ui::Dataspace::V0_SRGB, ui::PixelFormat::RGBA_8888, - Rect(), FRAME_SCALE, &outBuffer); + Rect(0, 0, 1, 1), FRAME_SCALE, &outBuffer); }; ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, NO_ERROR, PERMISSION_DENIED)); } diff --git a/services/surfaceflinger/tests/LayerUpdate_test.cpp b/services/surfaceflinger/tests/LayerUpdate_test.cpp index 73f563dfc7..0ad122b5ed 100644 --- a/services/surfaceflinger/tests/LayerUpdate_test.cpp +++ b/services/surfaceflinger/tests/LayerUpdate_test.cpp @@ -1443,6 +1443,61 @@ TEST_F(ScreenCaptureTest, CaptureRelativeInTree) { mCapture->expectColor(Rect(0, 0, 9, 9), {100, 100, 100, 255}); } +TEST_F(ScreenCaptureTest, CaptureBoundlessLayerWithSourceCrop) { + sp child = createColorLayer("Child layer", Color::RED, mFGSurfaceControl.get()); + SurfaceComposerClient::Transaction().show(child).apply(true); + + sp sf(ComposerService::getComposerService()); + sp outBuffer; + Rect sourceCrop(0, 0, 10, 10); + ASSERT_EQ(NO_ERROR, sf->captureLayers(child->getHandle(), &outBuffer, sourceCrop)); + ScreenCapture sc(outBuffer); + + sc.expectColor(Rect(0, 0, 9, 9), Color::RED); +} + +TEST_F(ScreenCaptureTest, CaptureBoundedLayerWithoutSourceCrop) { + sp child = createColorLayer("Child layer", Color::RED, mFGSurfaceControl.get()); + Rect layerCrop(0, 0, 10, 10); + SurfaceComposerClient::Transaction().setCrop_legacy(child, layerCrop).show(child).apply(true); + + sp sf(ComposerService::getComposerService()); + sp outBuffer; + Rect sourceCrop = Rect(); + ASSERT_EQ(NO_ERROR, sf->captureLayers(child->getHandle(), &outBuffer, sourceCrop)); + ScreenCapture sc(outBuffer); + + sc.expectColor(Rect(0, 0, 9, 9), Color::RED); +} + +TEST_F(ScreenCaptureTest, CaptureBoundlessLayerWithoutSourceCropFails) { + sp child = createColorLayer("Child layer", Color::RED, mFGSurfaceControl.get()); + SurfaceComposerClient::Transaction().show(child).apply(true); + + sp sf(ComposerService::getComposerService()); + sp outBuffer; + Rect sourceCrop = Rect(); + + ASSERT_EQ(BAD_VALUE, sf->captureLayers(child->getHandle(), &outBuffer, sourceCrop)); +} + +TEST_F(ScreenCaptureTest, CaptureBufferLayerWithoutBufferFails) { + sp child = createSurface(mClient, "Child surface", 10, 10, + PIXEL_FORMAT_RGBA_8888, 0, mFGSurfaceControl.get()); + SurfaceComposerClient::Transaction().show(child).apply(true); + + sp sf(ComposerService::getComposerService()); + sp outBuffer; + Rect sourceCrop = Rect(); + ASSERT_EQ(BAD_VALUE, sf->captureLayers(child->getHandle(), &outBuffer, sourceCrop)); + + TransactionUtils::fillSurfaceRGBA8(child, Color::RED); + SurfaceComposerClient::Transaction().apply(true); + ASSERT_EQ(NO_ERROR, sf->captureLayers(child->getHandle(), &outBuffer, sourceCrop)); + ScreenCapture sc(outBuffer); + sc.expectColor(Rect(0, 0, 9, 9), Color::RED); +} + // In the following tests we verify successful skipping of a parent layer, // so we use the same verification logic and only change how we mutate // the parent layer to verify that various properties are ignored. diff --git a/services/surfaceflinger/tests/utils/TransactionUtils.h b/services/surfaceflinger/tests/utils/TransactionUtils.h index 22df2559b1..2c63da0cdc 100644 --- a/services/surfaceflinger/tests/utils/TransactionUtils.h +++ b/services/surfaceflinger/tests/utils/TransactionUtils.h @@ -136,6 +136,11 @@ public: } } + static void fillSurfaceRGBA8(const sp& sc, const Color& color, + bool unlock = true) { + fillSurfaceRGBA8(sc, color.r, color.g, color.b, unlock); + } + // Fill an RGBA_8888 formatted surface with a single color. static void fillSurfaceRGBA8(const sp& sc, uint8_t r, uint8_t g, uint8_t b, bool unlock = true) { -- cgit v1.2.3-59-g8ed1b