From 2ac47153cf897bfe24eab0dd5aae3ed9fdf88df4 Mon Sep 17 00:00:00 2001 From: Peter Cai Date: Sun, 15 Jun 2025 17:36:10 -0400 Subject: [PATCH 3/6] Revert "Remove flag single_hop_screenshots" This reverts commit 16c4c32c55231dc241c386a0423710baa452b7de. Further changes have been made to SurfaceFlinger since that commit, for example, removal of displayState and attachGainmap. This revert has been adjusted accordingly, though not 100% sure everything is correct. Change-Id: Ibdea8e3dec6ad4b85c43ad059847c63f934c3d24 Conflicts: services/surfaceflinger/RegionSamplingThread.cpp services/surfaceflinger/SurfaceFlinger.cpp services/surfaceflinger/SurfaceFlinger.h services/surfaceflinger/common/FlagManager.cpp services/surfaceflinger/common/include/common/FlagManager.h services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h --- .../surfaceflinger/RegionSamplingThread.cpp | 19 +- services/surfaceflinger/SurfaceFlinger.cpp | 251 ++++++++++++------ services/surfaceflinger/SurfaceFlinger.h | 10 +- .../surfaceflinger/common/FlagManager.cpp | 2 + .../common/include/common/FlagManager.h | 1 + .../tests/unittests/TestableSurfaceFlinger.h | 2 +- 6 files changed, 195 insertions(+), 90 deletions(-) diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 615492a872..e266656b5d 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -355,11 +355,20 @@ void RegionSamplingThread::captureSample() { screenshotArgs.isSecure = true; screenshotArgs.seamlessTransition = false; - std::vector>> layers; - mFlinger.getSnapshotsFromMainThread(screenshotArgs, getLayerSnapshotsFn, layers); - FenceResult fenceResult = mFlinger.captureScreenshot(screenshotArgs, buffer, kRegionSampling, - kGrayscale, kIsProtected, nullptr, layers) - .get(); + FenceResult fenceResult; + if (FlagManager::getInstance().single_hop_screenshot()) { + std::vector>> layers; + auto displayState = + mFlinger.getSnapshotsFromMainThread(screenshotArgs, getLayerSnapshotsFn, layers); + fenceResult = mFlinger.captureScreenshot(screenshotArgs, buffer, kRegionSampling, + kGrayscale, kIsProtected, nullptr, layers) + .get(); + } else { + fenceResult = mFlinger.captureScreenshotLegacy(screenshotArgs, getLayerSnapshotsFn, buffer, + kRegionSampling, kGrayscale, + kIsProtected, false /* attachGainmap */, nullptr) + .get(); + } if (fenceResult.ok()) { fenceResult.value()->waitForever(LOG_TAG); } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 42a77fd844..d601b24cbd 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -7706,87 +7706,124 @@ void SurfaceFlinger::captureScreenCommon(ScreenshotArgs& args, return; } - std::vector>> layers; - bool hasDisplayState = getSnapshotsFromMainThread(args, getLayerSnapshotsFn, layers); - if (!hasDisplayState) { - ALOGD("Display state not found"); - invokeScreenCaptureError(NO_MEMORY, captureListener); - } + if (FlagManager::getInstance().single_hop_screenshot()) { + std::vector>> layers; + bool hasDisplayState = getSnapshotsFromMainThread(args, getLayerSnapshotsFn, layers); + if (!hasDisplayState) { + ALOGD("Display state not found"); + invokeScreenCaptureError(NO_MEMORY, captureListener); + } - const bool hasHdrLayer = std::any_of(layers.cbegin(), layers.cend(), [this](const auto& layer) { - return isHdrLayer(*(layer.second->mSnapshot.get())); - }); + const bool hasHdrLayer = std::any_of(layers.cbegin(), layers.cend(), [this](const auto& layer) { + return isHdrLayer(*(layer.second->mSnapshot.get())); + }); - 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); - - std::shared_ptr hdrTexture; - std::shared_ptr gainmapTexture; - - if (hasHdrLayer && !args.seamlessTransition && - FlagManager::getInstance().true_hdr_screenshots()) { - const auto hdrBuffer = - getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(), - HAL_PIXEL_FORMAT_RGBA_FP16, 1 /* layerCount */, - buffer->getUsage(), "screenshot-hdr"); - const auto gainmapBuffer = - getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(), - buffer->getPixelFormat(), 1 /* layerCount */, - buffer->getUsage(), "screenshot-gainmap"); - - const status_t hdrBufferStatus = hdrBuffer->initCheck(); - const status_t gainmapBufferStatus = gainmapBuffer->initCheck(); - - if (hdrBufferStatus != OK || gainmapBufferStatus != -OK) { - if (hdrBufferStatus != OK) { - ALOGW("%s: Buffer failed to allocate for hdr: %d. Screenshoting SDR instead.", - __func__, hdrBufferStatus); + 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); + + std::shared_ptr hdrTexture; + std::shared_ptr gainmapTexture; + + if (hasHdrLayer && !args.seamlessTransition && + FlagManager::getInstance().true_hdr_screenshots()) { + const auto hdrBuffer = + getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(), + HAL_PIXEL_FORMAT_RGBA_FP16, 1 /* layerCount */, + buffer->getUsage(), "screenshot-hdr"); + const auto gainmapBuffer = + getFactory().createGraphicBuffer(buffer->getWidth(), buffer->getHeight(), + buffer->getPixelFormat(), 1 /* layerCount */, + buffer->getUsage(), "screenshot-gainmap"); + + const status_t hdrBufferStatus = hdrBuffer->initCheck(); + const status_t gainmapBufferStatus = gainmapBuffer->initCheck(); + + if (hdrBufferStatus != OK || gainmapBufferStatus != -OK) { + if (hdrBufferStatus != OK) { + ALOGW("%s: Buffer failed to allocate for hdr: %d. Screenshoting SDR instead.", + __func__, hdrBufferStatus); + } else { + ALOGW("%s: Buffer failed to allocate for gainmap: %d. Screenshoting SDR instead.", + __func__, gainmapBufferStatus); + } } else { - ALOGW("%s: Buffer failed to allocate for gainmap: %d. Screenshoting SDR instead.", - __func__, gainmapBufferStatus); + hdrTexture = std::make_shared< + renderengine::impl::ExternalTexture>(hdrBuffer, getRenderEngine(), + renderengine::impl::ExternalTexture:: + Usage::WRITEABLE); + gainmapTexture = std::make_shared< + renderengine::impl::ExternalTexture>(gainmapBuffer, getRenderEngine(), + renderengine::impl::ExternalTexture:: + Usage::WRITEABLE); } - } else { - hdrTexture = std::make_shared< - renderengine::impl::ExternalTexture>(hdrBuffer, getRenderEngine(), - renderengine::impl::ExternalTexture:: - Usage::WRITEABLE); - gainmapTexture = std::make_shared< - renderengine::impl::ExternalTexture>(gainmapBuffer, getRenderEngine(), - renderengine::impl::ExternalTexture:: - Usage::WRITEABLE); } - } - auto futureFence = - captureScreenshot(args, texture, false /* regionSampling */, grayscale, isProtected, - captureListener, layers, hdrTexture, gainmapTexture); - futureFence.get(); + auto futureFence = + captureScreenshot(args, texture, false /* regionSampling */, grayscale, isProtected, + captureListener, layers, hdrTexture, gainmapTexture); + 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(args, getLayerSnapshotsFn, texture, + false /* regionSampling */, grayscale, + isProtected, false, captureListener); + futureFence.get(); + } } // Returns true if display is found and args was populated with display state @@ -7854,7 +7891,7 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( if (hdrBuffer && gainmapBuffer) { ftl::SharedFuture hdrRenderFuture = renderScreenImpl(args, hdrBuffer, regionSampling, grayscale, isProtected, - captureResults, layers); + true, captureResults, layers); captureResults.buffer = buffer->getBuffer(); captureResults.optionalGainMap = gainmapBuffer->getBuffer(); @@ -7876,7 +7913,7 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( .share(); } else { renderFuture = renderScreenImpl(args, buffer, regionSampling, grayscale, isProtected, - captureResults, layers); + false, captureResults, layers); } if (captureListener) { @@ -7894,10 +7931,54 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( return renderFuture; } +ftl::SharedFuture SurfaceFlinger::captureScreenshotLegacy( + ScreenshotArgs& args, GetLayerSnapshotsFunction getLayerSnapshotsFn, + const std::shared_ptr& buffer, bool regionSampling, + bool grayscale, bool isProtected, bool attachGainmap, + const sp& captureListener) { + SFTRACE_CALL(); + + auto takeScreenshotFn = [=, this, args = std::move(args)]() REQUIRES( + kMainThreadContext) mutable -> ftl::SharedFuture { + auto layers = getLayerSnapshotsFn(); + + ScreenCaptureResults captureResults; + + ftl::SharedFuture renderFuture = + renderScreenImpl(args, buffer, regionSampling, grayscale, isProtected, + attachGainmap, captureResults, 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( ScreenshotArgs& args, const std::shared_ptr& buffer, - bool regionSampling, bool grayscale, bool isProtected, ScreenCaptureResults& captureResults, - const std::vector>>& layers) { + bool regionSampling, bool grayscale, bool isProtected, bool attachGainmap, + ScreenCaptureResults& captureResults, const std::vector>>& layers) { SFTRACE_CALL(); for (auto& [_, layerFE] : layers) { @@ -8038,9 +8119,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 f0298079a0..88c1701998 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -943,10 +943,16 @@ private: const std::shared_ptr& hdrBuffer = nullptr, const std::shared_ptr& gainmapBuffer = nullptr); + ftl::SharedFuture captureScreenshotLegacy( + ScreenshotArgs& args, GetLayerSnapshotsFunction, + const std::shared_ptr&, bool regionSampling, + bool grayscale, bool isProtected, bool attachGainmap, + const sp&); + ftl::SharedFuture renderScreenImpl( ScreenshotArgs& args, const std::shared_ptr&, - bool regionSampling, bool grayscale, bool isProtected, ScreenCaptureResults&, - const std::vector>>& layers); + bool regionSampling, bool grayscale, bool isProtected, bool attachGainmap, + ScreenCaptureResults&, const std::vector>>& layers); void readPersistentProperties(); diff --git a/services/surfaceflinger/common/FlagManager.cpp b/services/surfaceflinger/common/FlagManager.cpp index ebf451501d..6f3a58f6ff 100644 --- a/services/surfaceflinger/common/FlagManager.cpp +++ b/services/surfaceflinger/common/FlagManager.cpp @@ -172,6 +172,7 @@ void FlagManager::dump(std::string& result) const { DUMP_ACONFIG_FLAG(reject_dupe_layerstacks); DUMP_ACONFIG_FLAG(renderable_buffer_usage); DUMP_ACONFIG_FLAG(restore_blur_step); + DUMP_ACONFIG_FLAG(single_hop_screenshot); DUMP_ACONFIG_FLAG(skip_invisible_windows_in_input); DUMP_ACONFIG_FLAG(stable_edid_ids); DUMP_ACONFIG_FLAG(synced_resolution_switch); @@ -290,6 +291,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 72b3bc302a..23232c381f 100644 --- a/services/surfaceflinger/common/include/common/FlagManager.h +++ b/services/surfaceflinger/common/include/common/FlagManager.h @@ -107,6 +107,7 @@ public: bool reject_dupe_layerstacks() const; bool renderable_buffer_usage() const; bool restore_blur_step() const; + bool single_hop_screenshot() const; bool skip_invisible_windows_in_input() const; bool stable_edid_ids() const; bool synced_resolution_switch() const; diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 13c32bdf08..7e519d3842 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -494,7 +494,7 @@ public: return mFlinger->renderScreenImpl(screenshotArgs, buffer, regionSampling, false /* grayscale */, false /* isProtected */, - captureResults, layers); + false /*attachGainmap */, captureResults, layers); } auto getLayerSnapshotsForScreenshotsFn(ui::LayerStack layerStack, uint32_t uid) { -- 2.48.1