From b12faaf3064de58caf0f6bfaf7fcac5a47f5024d Mon Sep 17 00:00:00 2001 From: Peter Cai Date: Sun, 16 Mar 2025 12:54:56 -0400 Subject: [PATCH 4/7] Revert "Remove flag single_hop_screenshots" This reverts commit 16c4c32c55231dc241c386a0423710baa452b7de. Change-Id: Ib1618cd4e1f83ba1f211a04429538b32e8de79da --- .../surfaceflinger/RegionSamplingThread.cpp | 22 ++- services/surfaceflinger/SurfaceFlinger.cpp | 184 ++++++++++++++---- services/surfaceflinger/SurfaceFlinger.h | 10 +- .../surfaceflinger/common/FlagManager.cpp | 2 + .../common/include/common/FlagManager.h | 1 + .../tests/unittests/TestableSurfaceFlinger.h | 3 +- 6 files changed, 171 insertions(+), 51 deletions(-) diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 21d3396ebe..08f831c8e5 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -353,13 +353,21 @@ void RegionSamplingThread::captureSample() { sampledBounds.getSize(), ui::Dataspace::V0_SRGB, displayWeak, RenderArea::Options::CAPTURE_SECURE_LAYERS); - std::vector>> layers; - auto displayState = - mFlinger.getSnapshotsFromMainThread(renderAreaBuilder, getLayerSnapshotsFn, layers); - FenceResult fenceResult = - mFlinger.captureScreenshot(renderAreaBuilder, buffer, kRegionSampling, kGrayscale, - kIsProtected, kAttachGainmap, nullptr, displayState, layers) - .get(); + FenceResult fenceResult; + if (FlagManager::getInstance().single_hop_screenshot()) { + std::vector>> layers; + auto displayState = + mFlinger.getSnapshotsFromMainThread(renderAreaBuilder, getLayerSnapshotsFn, layers); + fenceResult = mFlinger.captureScreenshot(renderAreaBuilder, buffer, kRegionSampling, + kGrayscale, kIsProtected, kAttachGainmap, nullptr, + displayState, layers) + .get(); + } else { + fenceResult = mFlinger.captureScreenshotLegacy(renderAreaBuilder, getLayerSnapshotsFn, + buffer, kRegionSampling, kGrayscale, + kIsProtected, kAttachGainmap, nullptr) + .get(); + } if (fenceResult.ok()) { fenceResult.value()->waitForever(LOG_TAG); } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 896d13d31d..852a8b81e1 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -7389,41 +7389,79 @@ void SurfaceFlinger::captureScreenCommon(RenderAreaBuilderVariant renderAreaBuil return; } - std::vector>> layers; - auto displayState = getSnapshotsFromMainThread(renderAreaBuilder, getLayerSnapshotsFn, layers); - - const bool supportsProtected = getRenderEngine().supportsProtectedContent(); - bool hasProtectedLayer = false; - if (allowProtected && supportsProtected) { - hasProtectedLayer = layersHasProtectedLayer(layers); - } - const bool isProtected = hasProtectedLayer && allowProtected && supportsProtected; - const uint32_t usage = GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_RENDER | - GRALLOC_USAGE_HW_TEXTURE | - (isProtected ? GRALLOC_USAGE_PROTECTED - : GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN); - sp buffer = - getFactory().createGraphicBuffer(bufferSize.getWidth(), bufferSize.getHeight(), - static_cast(reqPixelFormat), - 1 /* layerCount */, usage, "screenshot"); - - const status_t bufferStatus = buffer->initCheck(); - if (bufferStatus != OK) { - // Animations may end up being really janky, but don't crash here. - // Otherwise an irreponsible process may cause an SF crash by allocating - // too much. - ALOGE("%s: Buffer failed to allocate: %d", __func__, bufferStatus); - invokeScreenCaptureError(bufferStatus, captureListener); - return; + if (FlagManager::getInstance().single_hop_screenshot()) { + std::vector>> layers; + auto displayState = + getSnapshotsFromMainThread(renderAreaBuilder, getLayerSnapshotsFn, layers); + + const bool supportsProtected = getRenderEngine().supportsProtectedContent(); + bool hasProtectedLayer = false; + if (allowProtected && supportsProtected) { + hasProtectedLayer = layersHasProtectedLayer(layers); + } + const bool isProtected = hasProtectedLayer && allowProtected && supportsProtected; + const uint32_t usage = GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_RENDER | + GRALLOC_USAGE_HW_TEXTURE | + (isProtected ? GRALLOC_USAGE_PROTECTED + : GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN); + sp buffer = + getFactory().createGraphicBuffer(bufferSize.getWidth(), bufferSize.getHeight(), + static_cast(reqPixelFormat), + 1 /* layerCount */, usage, "screenshot"); + + const status_t bufferStatus = buffer->initCheck(); + if (bufferStatus != OK) { + // Animations may end up being really janky, but don't crash here. + // Otherwise an irreponsible process may cause an SF crash by allocating + // too much. + ALOGE("%s: Buffer failed to allocate: %d", __func__, bufferStatus); + invokeScreenCaptureError(bufferStatus, captureListener); + return; + } + const std::shared_ptr texture = std::make_shared< + renderengine::impl::ExternalTexture>(buffer, getRenderEngine(), + renderengine::impl::ExternalTexture::Usage:: + WRITEABLE); + auto futureFence = captureScreenshot(renderAreaBuilder, texture, false /* regionSampling */, + grayscale, isProtected, attachGainmap, captureListener, + displayState, layers); + futureFence.get(); + + } else { + const bool supportsProtected = getRenderEngine().supportsProtectedContent(); + bool hasProtectedLayer = false; + if (allowProtected && supportsProtected) { + auto layers = mScheduler->schedule([=]() { return getLayerSnapshotsFn(); }).get(); + hasProtectedLayer = layersHasProtectedLayer(layers); + } + const bool isProtected = hasProtectedLayer && allowProtected && supportsProtected; + const uint32_t usage = GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_RENDER | + GRALLOC_USAGE_HW_TEXTURE | + (isProtected ? GRALLOC_USAGE_PROTECTED + : GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN); + sp buffer = + getFactory().createGraphicBuffer(bufferSize.getWidth(), bufferSize.getHeight(), + static_cast(reqPixelFormat), + 1 /* layerCount */, usage, "screenshot"); + + const status_t bufferStatus = buffer->initCheck(); + if (bufferStatus != OK) { + // Animations may end up being really janky, but don't crash here. + // Otherwise an irreponsible process may cause an SF crash by allocating + // too much. + ALOGE("%s: Buffer failed to allocate: %d", __func__, bufferStatus); + invokeScreenCaptureError(bufferStatus, captureListener); + return; + } + const std::shared_ptr texture = std::make_shared< + renderengine::impl::ExternalTexture>(buffer, getRenderEngine(), + renderengine::impl::ExternalTexture::Usage:: + WRITEABLE); + auto futureFence = captureScreenshotLegacy(renderAreaBuilder, getLayerSnapshotsFn, texture, + false /* regionSampling */, grayscale, + isProtected, attachGainmap, captureListener); + futureFence.get(); } - const std::shared_ptr texture = std::make_shared< - renderengine::impl::ExternalTexture>(buffer, getRenderEngine(), - renderengine::impl::ExternalTexture::Usage:: - WRITEABLE); - auto futureFence = - captureScreenshot(renderAreaBuilder, texture, false /* regionSampling */, grayscale, - isProtected, attachGainmap, captureListener, displayState, layers); - futureFence.get(); } std::optional @@ -7489,7 +7527,7 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( ftl::SharedFuture renderFuture = renderScreenImpl(renderArea.get(), buffer, regionSampling, grayscale, isProtected, - captureResults, displayState, layers); + attachGainmap, captureResults, displayState, layers); if (captureResults.capturedHdrLayers && attachGainmap && FlagManager::getInstance().true_hdr_screenshots()) { @@ -7524,7 +7562,8 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( ScreenCaptureResults unusedResults; ftl::SharedFuture hdrRenderFuture = renderScreenImpl(renderArea.get(), hdrTexture, regionSampling, grayscale, - isProtected, unusedResults, displayState, layers); + isProtected, attachGainmap, unusedResults, displayState, + layers); renderFuture = ftl::Future(std::move(renderFuture)) @@ -7570,10 +7609,67 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( return renderFuture; } +ftl::SharedFuture SurfaceFlinger::captureScreenshotLegacy( + RenderAreaBuilderVariant renderAreaBuilder, GetLayerSnapshotsFunction getLayerSnapshotsFn, + const std::shared_ptr& buffer, bool regionSampling, + bool grayscale, bool isProtected, bool attachGainmap, + const sp& captureListener) { + SFTRACE_CALL(); + + auto takeScreenshotFn = [=, this, renderAreaBuilder = std::move(renderAreaBuilder)]() REQUIRES( + kMainThreadContext) mutable -> ftl::SharedFuture { + auto layers = getLayerSnapshotsFn(); + auto displayState = getDisplayStateFromRenderAreaBuilder(renderAreaBuilder); + + ScreenCaptureResults captureResults; + std::unique_ptr renderArea = + std::visit([](auto&& arg) -> std::unique_ptr { return arg.build(); }, + renderAreaBuilder); + + if (!renderArea) { + ALOGW("Skipping screen capture because of invalid render area."); + if (captureListener) { + captureResults.fenceResult = base::unexpected(NO_MEMORY); + captureListener->onScreenCaptureCompleted(captureResults); + } + return ftl::yield(base::unexpected(NO_ERROR)).share(); + } + + ftl::SharedFuture renderFuture = + renderScreenImpl(renderArea.get(), buffer, regionSampling, grayscale, isProtected, + attachGainmap, captureResults, displayState, layers); + + if (captureListener) { + // Defer blocking on renderFuture back to the Binder thread. + return ftl::Future(std::move(renderFuture)) + .then([captureListener, captureResults = std::move(captureResults)]( + FenceResult fenceResult) mutable -> FenceResult { + captureResults.fenceResult = std::move(fenceResult); + captureListener->onScreenCaptureCompleted(captureResults); + return base::unexpected(NO_ERROR); + }) + .share(); + } + return renderFuture; + }; + + // TODO(b/294936197): Run takeScreenshotsFn() in a binder thread to reduce the number + // of calls on the main thread. + auto future = + mScheduler->schedule(FTL_FAKE_GUARD(kMainThreadContext, std::move(takeScreenshotFn))); + + // Flatten nested futures. + auto chain = ftl::Future(std::move(future)).then([](ftl::SharedFuture future) { + return future; + }); + + return chain.share(); +} + ftl::SharedFuture SurfaceFlinger::renderScreenImpl( const RenderArea* renderArea, const std::shared_ptr& buffer, - bool regionSampling, bool grayscale, bool isProtected, ScreenCaptureResults& captureResults, - std::optional& displayState, + bool regionSampling, bool grayscale, bool isProtected, bool attachGainmap, + ScreenCaptureResults& captureResults, std::optional& displayState, std::vector>>& layers) { SFTRACE_CALL(); @@ -7719,9 +7815,15 @@ ftl::SharedFuture SurfaceFlinger::renderScreenImpl( // // TODO(b/196334700) Once we use RenderEngineThreaded everywhere we can always defer the call // to CompositionEngine::present. - ftl::SharedFuture presentFuture = mRenderEngine->isThreaded() - ? ftl::yield(present()).share() - : mScheduler->schedule(std::move(present)).share(); + ftl::SharedFuture presentFuture; + if (FlagManager::getInstance().single_hop_screenshot()) { + presentFuture = mRenderEngine->isThreaded() + ? ftl::yield(present()).share() + : mScheduler->schedule(std::move(present)).share(); + } else { + presentFuture = mRenderEngine->isThreaded() ? ftl::defer(std::move(present)).share() + : ftl::yield(present()).share(); + } return presentFuture; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 1e2c08747b..835cb572b4 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -885,10 +885,16 @@ private: std::optional& displayState, std::vector>>& layers); + ftl::SharedFuture captureScreenshotLegacy( + RenderAreaBuilderVariant, GetLayerSnapshotsFunction, + const std::shared_ptr&, bool regionSampling, + bool grayscale, bool isProtected, bool attachGainmap, + const sp&); + ftl::SharedFuture renderScreenImpl( const RenderArea*, const std::shared_ptr&, - bool regionSampling, bool grayscale, bool isProtected, ScreenCaptureResults&, - std::optional& displayState, + bool regionSampling, bool grayscale, bool isProtected, bool attachGainmap, + ScreenCaptureResults&, std::optional& displayState, std::vector>>& layers); void readPersistentProperties(); diff --git a/services/surfaceflinger/common/FlagManager.cpp b/services/surfaceflinger/common/FlagManager.cpp index 5e78426c77..5c417ba748 100644 --- a/services/surfaceflinger/common/FlagManager.cpp +++ b/services/surfaceflinger/common/FlagManager.cpp @@ -166,6 +166,7 @@ void FlagManager::dump(std::string& result) const { DUMP_ACONFIG_FLAG(skip_invisible_windows_in_input); DUMP_ACONFIG_FLAG(begone_bright_hlg); DUMP_ACONFIG_FLAG(window_blur_kawase2); + DUMP_ACONFIG_FLAG(single_hop_screenshot); #undef DUMP_ACONFIG_FLAG #undef DUMP_LEGACY_SERVER_FLAG @@ -259,6 +260,7 @@ FLAG_MANAGER_ACONFIG_FLAG(local_tonemap_screenshots, "debug.sf.local_tonemap_scr FLAG_MANAGER_ACONFIG_FLAG(override_trusted_overlay, ""); FLAG_MANAGER_ACONFIG_FLAG(flush_buffer_slots_to_uncache, ""); FLAG_MANAGER_ACONFIG_FLAG(force_compile_graphite_renderengine, ""); +FLAG_MANAGER_ACONFIG_FLAG(single_hop_screenshot, ""); FLAG_MANAGER_ACONFIG_FLAG(true_hdr_screenshots, "debug.sf.true_hdr_screenshots"); FLAG_MANAGER_ACONFIG_FLAG(display_config_error_hal, ""); FLAG_MANAGER_ACONFIG_FLAG(connected_display_hdr, "debug.sf.connected_display_hdr"); diff --git a/services/surfaceflinger/common/include/common/FlagManager.h b/services/surfaceflinger/common/include/common/FlagManager.h index d8887f538f..15284ba0ee 100644 --- a/services/surfaceflinger/common/include/common/FlagManager.h +++ b/services/surfaceflinger/common/include/common/FlagManager.h @@ -95,6 +95,7 @@ public: bool override_trusted_overlay() const; bool flush_buffer_slots_to_uncache() const; bool force_compile_graphite_renderengine() const; + bool single_hop_screenshot() const; bool trace_frame_rate_override() const; bool true_hdr_screenshots() const; bool display_config_error_hal() const; diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 7f0b7a6585..fd8bc0d22d 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -475,7 +475,8 @@ public: return mFlinger->renderScreenImpl(renderArea.get(), buffer, regionSampling, false /* grayscale */, false /* isProtected */, - captureResults, displayState, layers); + false /* attachGainmap */, captureResults, displayState, + layers); } auto getLayerSnapshotsForScreenshotsFn(ui::LayerStack layerStack, uint32_t uid) { -- 2.48.1