From 0aeb91404524322d4b1ccd0eeb76e5b2f735691f Mon Sep 17 00:00:00 2001 From: Peter Cai Date: Sun, 15 Jun 2025 18:31:42 -0400 Subject: [PATCH 4/6] Revert "Reorder release fence attachment for non-threaded RE" This reverts commit 0b80c74300b73e937ee9a9cb58487bd126daa4d8. Change-Id: I7974588c1ca5c129b81846fbfb5a3422f4f98cf5 Conflicts: services/surfaceflinger/RegionSamplingThread.cpp services/surfaceflinger/SurfaceFlinger.cpp services/surfaceflinger/SurfaceFlinger.h services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h --- .../surfaceflinger/RegionSamplingThread.cpp | 9 +- services/surfaceflinger/SurfaceFlinger.cpp | 104 ++++++++++-------- services/surfaceflinger/SurfaceFlinger.h | 15 ++- .../tests/unittests/TestableSurfaceFlinger.h | 3 +- 4 files changed, 74 insertions(+), 57 deletions(-) diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index e266656b5d..7e2144d6b1 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -356,12 +356,13 @@ void RegionSamplingThread::captureSample() { screenshotArgs.seamlessTransition = false; FenceResult fenceResult; - if (FlagManager::getInstance().single_hop_screenshot()) { - std::vector>> layers; + if (FlagManager::getInstance().single_hop_screenshot() && + mFlinger.mRenderEngine->isThreaded()) { + std::vector> layerFEs; auto displayState = - mFlinger.getSnapshotsFromMainThread(screenshotArgs, getLayerSnapshotsFn, layers); + mFlinger.getSnapshotsFromMainThread(screenshotArgs, getLayerSnapshotsFn, layerFEs); fenceResult = mFlinger.captureScreenshot(screenshotArgs, buffer, kRegionSampling, - kGrayscale, kIsProtected, nullptr, layers) + kGrayscale, kIsProtected, nullptr, layerFEs) .get(); } else { fenceResult = mFlinger.captureScreenshotLegacy(screenshotArgs, getLayerSnapshotsFn, buffer, diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index d601b24cbd..235ddfb762 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -7650,10 +7650,9 @@ void SurfaceFlinger::attachReleaseFenceFutureToLayer(Layer* layer, LayerFE* laye // typically a layer with DRM contents, or have the GRALLOC_USAGE_PROTECTED set on the buffer. // A protected layer has no implication on whether it's secure, which is explicitly set by // application to avoid being screenshot or drawn via unsecure display. -bool SurfaceFlinger::layersHasProtectedLayer( - const std::vector>>& layers) const { +bool SurfaceFlinger::layersHasProtectedLayer(const std::vector>& layers) const { bool protectedLayerFound = false; - for (auto& [_, layerFE] : layers) { + for (auto& layerFE : layers) { protectedLayerFound |= (layerFE->mSnapshot->isVisible && layerFE->mSnapshot->hasProtectedContent); if (protectedLayerFound) { @@ -7669,23 +7668,17 @@ bool SurfaceFlinger::layersHasProtectedLayer( // risk of deadlocks. Returns false if no display is found. bool SurfaceFlinger::getSnapshotsFromMainThread( ScreenshotArgs& args, GetLayerSnapshotsFunction getLayerSnapshotsFn, - std::vector>>& layers) { + std::vector>& layerFEs) { return mScheduler - ->schedule([=, this, &args, &layers]() REQUIRES(kMainThreadContext) { + ->schedule([=, this, &args, &layerFEs]() REQUIRES(kMainThreadContext) { SFTRACE_NAME_FOR_TRACK(WorkloadTracer::TRACK_NAME, "Screenshot"); mPowerAdvisor->setScreenshotWorkload(); SFTRACE_NAME("getSnapshotsFromMainThread"); - layers = getLayerSnapshotsFn(); - // Non-threaded RenderEngine eventually returns to the main thread a 2nd time - // to complete the screenshot. Release fences should only be added during the 2nd - // hop to main thread in order to avoid potential deadlocks from waiting for the - // the future fence to fire. - if (mRenderEngine->isThreaded()) { - for (auto& [layer, layerFE] : layers) { - attachReleaseFenceFutureToLayer(layer, layerFE.get(), - ui::INVALID_LAYER_STACK); - } + auto layers = getLayerSnapshotsFn(); + for (auto& [layer, layerFE] : layers) { + attachReleaseFenceFutureToLayer(layer, layerFE.get(), ui::INVALID_LAYER_STACK); } + layerFEs = extractLayerFEs(layers); return getDisplayStateOnMainThread(args); }) .get(); @@ -7706,22 +7699,22 @@ void SurfaceFlinger::captureScreenCommon(ScreenshotArgs& args, return; } - if (FlagManager::getInstance().single_hop_screenshot()) { - std::vector>> layers; - bool hasDisplayState = getSnapshotsFromMainThread(args, getLayerSnapshotsFn, layers); + if (FlagManager::getInstance().single_hop_screenshot() && mRenderEngine->isThreaded()) { + std::vector> layerFEs; + bool hasDisplayState = getSnapshotsFromMainThread(args, getLayerSnapshotsFn, layerFEs); 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(layerFEs.cbegin(), layerFEs.cend(), [this](const auto& layer) { + return isHdrLayer(*(layer->mSnapshot.get())); }); const bool supportsProtected = getRenderEngine().supportsProtectedContent(); bool hasProtectedLayer = false; if (allowProtected && supportsProtected) { - hasProtectedLayer = layersHasProtectedLayer(layers); + hasProtectedLayer = layersHasProtectedLayer(layerFEs); } const bool isProtected = hasProtectedLayer && allowProtected && supportsProtected; const uint32_t usage = GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_RENDER | @@ -7787,14 +7780,14 @@ void SurfaceFlinger::captureScreenCommon(ScreenshotArgs& args, auto futureFence = captureScreenshot(args, texture, false /* regionSampling */, grayscale, isProtected, - captureListener, layers, hdrTexture, gainmapTexture); + captureListener, layerFEs, 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); + hasProtectedLayer = layersHasProtectedLayer(extractLayerFEs(layers)); } const bool isProtected = hasProtectedLayer && allowProtected && supportsProtected; const uint32_t usage = GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_RENDER | @@ -7874,11 +7867,21 @@ bool SurfaceFlinger::getDisplayStateOnMainThread(ScreenshotArgs& args) { return false; } +std::vector> SurfaceFlinger::extractLayerFEs( + const std::vector>>& layers) const { + std::vector> layerFEs; + layerFEs.reserve(layers.size()); + for (const auto& [_, layerFE] : layers) { + layerFEs.push_back(layerFE); + } + return layerFEs; +} + ftl::SharedFuture SurfaceFlinger::captureScreenshot( ScreenshotArgs& args, const std::shared_ptr& buffer, bool regionSampling, bool grayscale, bool isProtected, const sp& captureListener, - const std::vector>>& layers, + std::vector>& layerFEs, const std::shared_ptr& hdrBuffer, const std::shared_ptr& gainmapBuffer) { SFTRACE_CALL(); @@ -7888,10 +7891,13 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( float hdrSdrRatio = args.displayBrightnessNits / args.sdrWhitePointNits; + // Empty vector needed to pass into renderScreenImpl for legacy path + std::vector>> layers; + if (hdrBuffer && gainmapBuffer) { ftl::SharedFuture hdrRenderFuture = renderScreenImpl(args, hdrBuffer, regionSampling, grayscale, isProtected, - true, captureResults, layers); + true, captureResults, layers, layerFEs); captureResults.buffer = buffer->getBuffer(); captureResults.optionalGainMap = gainmapBuffer->getBuffer(); @@ -7913,7 +7919,7 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshot( .share(); } else { renderFuture = renderScreenImpl(args, buffer, regionSampling, grayscale, isProtected, - false, captureResults, layers); + false, captureResults, layers, layerFEs); } if (captureListener) { @@ -7942,11 +7948,16 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshotLegacy( kMainThreadContext) mutable -> ftl::SharedFuture { auto layers = getLayerSnapshotsFn(); + for (auto& [layer, layerFE] : layers) { + attachReleaseFenceFutureToLayer(layer, layerFE.get(), ui::INVALID_LAYER_STACK); + } + ScreenCaptureResults captureResults; + auto layerFEs = extractLayerFEs(layers); ftl::SharedFuture renderFuture = renderScreenImpl(args, buffer, regionSampling, grayscale, isProtected, - attachGainmap, captureResults, layers); + attachGainmap, captureResults, layers, layerFEs); if (captureListener) { // Defer blocking on renderFuture back to the Binder thread. @@ -7978,10 +7989,11 @@ ftl::SharedFuture SurfaceFlinger::captureScreenshotLegacy( ftl::SharedFuture SurfaceFlinger::renderScreenImpl( ScreenshotArgs& args, const std::shared_ptr& buffer, bool regionSampling, bool grayscale, bool isProtected, bool attachGainmap, - ScreenCaptureResults& captureResults, const std::vector>>& layers) { + ScreenCaptureResults& captureResults, std::vector>>& layers, + std::vector>& layerFEs) { SFTRACE_CALL(); - for (auto& [_, layerFE] : layers) { + for (auto& layerFE : layerFEs) { frontend::LayerSnapshot* snapshot = layerFE->mSnapshot.get(); captureResults.capturedSecureLayers |= (snapshot->isVisible && snapshot->isSecure); captureResults.capturedHdrLayers |= isHdrLayer(*snapshot); @@ -8026,31 +8038,29 @@ ftl::SharedFuture SurfaceFlinger::renderScreenImpl( captureResults.buffer = capturedBuffer->getBuffer(); ui::LayerStack layerStack{ui::DEFAULT_LAYER_STACK}; - if (!layers.empty()) { - const sp& layerFE = layers.back().second; + if (!layerFEs.empty()) { + const sp& layerFE = layerFEs.back(); layerStack = layerFE->getCompositionState()->outputFilter.layerStack; } + auto copyLayerFEs = [&layerFEs]() { + std::vector> ceLayerFEs; + ceLayerFEs.reserve(layerFEs.size()); + for (const auto& layerFE : layerFEs) { + ceLayerFEs.push_back(layerFE); + } + return ceLayerFEs; + }; + auto present = [this, buffer = capturedBuffer, dataspace = captureResults.capturedDataspace, - grayscale, isProtected, layers, layerStack, regionSampling, args, renderIntent, + grayscale, isProtected, layers, layerFEs = copyLayerFEs(), + layerStack, regionSampling, args, renderIntent, enableLocalTonemapping]() -> FenceResult { std::unique_ptr compositionEngine = mFactory.createCompositionEngine(); compositionEngine->setRenderEngine(mRenderEngine.get()); compositionEngine->setHwComposer(mHWComposer.get()); - std::vector> layerFEs; - layerFEs.reserve(layers.size()); - for (auto& [layer, layerFE] : layers) { - // Release fences were not yet added for non-threaded render engine. To avoid - // deadlocks between main thread and binder threads waiting for the future fence - // result, fences should be added to layers in the same hop onto the main thread. - if (!mRenderEngine->isThreaded()) { - attachReleaseFenceFutureToLayer(layer, layerFE.get(), ui::INVALID_LAYER_STACK); - } - layerFEs.push_back(layerFE); - } - compositionengine::Output::ColorProfile colorProfile{.dataspace = dataspace, .renderIntent = renderIntent}; @@ -8120,10 +8130,8 @@ ftl::SharedFuture SurfaceFlinger::renderScreenImpl( // TODO(b/196334700) Once we use RenderEngineThreaded everywhere we can always defer the call // to CompositionEngine::present. ftl::SharedFuture presentFuture; - if (FlagManager::getInstance().single_hop_screenshot()) { - presentFuture = mRenderEngine->isThreaded() - ? ftl::yield(present()).share() - : mScheduler->schedule(std::move(present)).share(); + if (FlagManager::getInstance().single_hop_screenshot() && mRenderEngine->isThreaded()) { + presentFuture = ftl::yield(present()).share(); } else { presentFuture = mRenderEngine->isThreaded() ? ftl::defer(std::move(present)).share() : ftl::yield(present()).share(); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 88c1701998..260fe20620 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -872,7 +872,7 @@ private: void attachReleaseFenceFutureToLayer(Layer* layer, LayerFE* layerFE, ui::LayerStack layerStack); // Checks if a protected layer exists in a list of layers. - bool layersHasProtectedLayer(const std::vector>>& layers) const; + bool layersHasProtectedLayer(const std::vector>& layers) const; using OutputCompositionState = compositionengine::impl::OutputCompositionState; @@ -927,7 +927,7 @@ private: bool getSnapshotsFromMainThread(ScreenshotArgs& args, GetLayerSnapshotsFunction getLayerSnapshotsFn, - std::vector>>& layers); + std::vector>& layerFEs); void captureScreenCommon(ScreenshotArgs& args, GetLayerSnapshotsFunction, ui::Size bufferSize, ui::PixelFormat, bool allowProtected, bool grayscale, @@ -935,11 +935,17 @@ private: bool getDisplayStateOnMainThread(ScreenshotArgs& args) REQUIRES(kMainThreadContext); + // Legacy layer raw pointer is not safe to access outside the main thread. + // Creates a new vector consisting only of LayerFEs, which can be safely + // accessed outside the main thread. + std::vector> extractLayerFEs( + const std::vector>>& layers) const; + ftl::SharedFuture captureScreenshot( ScreenshotArgs& args, const std::shared_ptr& buffer, bool regionSampling, bool grayscale, bool isProtected, const sp& captureListener, - const std::vector>>& layers, + std::vector>& layerFEs, const std::shared_ptr& hdrBuffer = nullptr, const std::shared_ptr& gainmapBuffer = nullptr); @@ -952,7 +958,8 @@ private: ftl::SharedFuture renderScreenImpl( ScreenshotArgs& args, const std::shared_ptr&, bool regionSampling, bool grayscale, bool isProtected, bool attachGainmap, - ScreenCaptureResults&, const std::vector>>& layers); + ScreenCaptureResults&, std::vector>>& layers, + std::vector>& layerFEs); void readPersistentProperties(); diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 7e519d3842..062df4d5e2 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -478,6 +478,7 @@ public: ScreenCaptureResults captureResults; const auto& state = display->getCompositionDisplay()->getState(); auto layers = getLayerSnapshotsFn(); + auto layerFEs = mFlinger->extractLayerFEs(layers); SurfaceFlinger::ScreenshotArgs screenshotArgs; screenshotArgs.captureTypeVariant = display; @@ -494,7 +495,7 @@ public: return mFlinger->renderScreenImpl(screenshotArgs, buffer, regionSampling, false /* grayscale */, false /* isProtected */, - false /*attachGainmap */, captureResults, layers); + false /*attachGainmap */, captureResults, layers, layerFEs); } auto getLayerSnapshotsForScreenshotsFn(ui::LayerStack layerStack, uint32_t uid) { -- 2.48.1