From 99599940f68158aeef735d8763980d0dbd397b51 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 27 Jan 2021 20:27:23 -0800 Subject: [PATCH] SurfaceFlinger: move sync_wait for screen capture to client Free up time from the SF's main thread by moving the fence waiting to the client. Test: Observe systrace of region sample thread Test: adb shell screencap Test: Recents takes the screenshot Test: Rotate device Test: Volume + power down for screenshot Bug: 178649983 Change-Id: I0a4991c013375b1f354e0728a06ca30a835b0422 --- libs/gui/ScreenCaptureResults.cpp | 15 +++++++++++++ .../android/gui/IScreenCaptureListener.aidl | 2 +- libs/gui/include/gui/ScreenCaptureResults.h | 2 ++ .../include/gui/SyncScreenCaptureListener.h | 6 ++++-- services/surfaceflinger/SurfaceFlinger.cpp | 21 +++++++------------ services/surfaceflinger/SurfaceFlinger.h | 4 ++-- .../surfaceflinger/tests/LayerState_test.cpp | 2 ++ .../tests/unittests/CompositionTest.cpp | 7 +------ .../tests/unittests/TestableSurfaceFlinger.h | 4 ++-- 9 files changed, 37 insertions(+), 26 deletions(-) diff --git a/libs/gui/ScreenCaptureResults.cpp b/libs/gui/ScreenCaptureResults.cpp index 2b29487268..f3849bcdcd 100644 --- a/libs/gui/ScreenCaptureResults.cpp +++ b/libs/gui/ScreenCaptureResults.cpp @@ -25,6 +25,14 @@ status_t ScreenCaptureResults::writeToParcel(android::Parcel* parcel) const { } else { SAFE_PARCEL(parcel->writeBool, false); } + + if (fence != Fence::NO_FENCE) { + SAFE_PARCEL(parcel->writeBool, true); + SAFE_PARCEL(parcel->write, *fence); + } else { + SAFE_PARCEL(parcel->writeBool, false); + } + SAFE_PARCEL(parcel->writeBool, capturedSecureLayers); SAFE_PARCEL(parcel->writeUint32, static_cast(capturedDataspace)); SAFE_PARCEL(parcel->writeInt32, result); @@ -39,6 +47,13 @@ status_t ScreenCaptureResults::readFromParcel(const android::Parcel* parcel) { SAFE_PARCEL(parcel->read, *buffer); } + bool hasFence; + SAFE_PARCEL(parcel->readBool, &hasFence); + if (hasFence) { + fence = new Fence(); + SAFE_PARCEL(parcel->read, *fence); + } + SAFE_PARCEL(parcel->readBool, &capturedSecureLayers); uint32_t dataspace = 0; SAFE_PARCEL(parcel->readUint32, &dataspace); diff --git a/libs/gui/aidl/android/gui/IScreenCaptureListener.aidl b/libs/gui/aidl/android/gui/IScreenCaptureListener.aidl index f490118b8f..e0f66cd0a1 100644 --- a/libs/gui/aidl/android/gui/IScreenCaptureListener.aidl +++ b/libs/gui/aidl/android/gui/IScreenCaptureListener.aidl @@ -20,5 +20,5 @@ import android.gui.ScreenCaptureResults; /** @hide */ oneway interface IScreenCaptureListener { - void onScreenCaptureComplete(in ScreenCaptureResults captureResults); + void onScreenCaptureCompleted(in ScreenCaptureResults captureResults); } \ No newline at end of file diff --git a/libs/gui/include/gui/ScreenCaptureResults.h b/libs/gui/include/gui/ScreenCaptureResults.h index fdb4b6985e..0ccc6e8b8a 100644 --- a/libs/gui/include/gui/ScreenCaptureResults.h +++ b/libs/gui/include/gui/ScreenCaptureResults.h @@ -27,6 +27,7 @@ #include #include +#include #include namespace android::gui { @@ -39,6 +40,7 @@ struct ScreenCaptureResults : public Parcelable { status_t readFromParcel(const android::Parcel* parcel) override; sp buffer; + sp fence = Fence::NO_FENCE; bool capturedSecureLayers{false}; ui::Dataspace capturedDataspace{ui::Dataspace::V0_SRGB}; status_t result = OK; diff --git a/libs/gui/include/gui/SyncScreenCaptureListener.h b/libs/gui/include/gui/SyncScreenCaptureListener.h index 8620b77c18..0784fbc058 100644 --- a/libs/gui/include/gui/SyncScreenCaptureListener.h +++ b/libs/gui/include/gui/SyncScreenCaptureListener.h @@ -26,14 +26,16 @@ using gui::ScreenCaptureResults; struct SyncScreenCaptureListener : gui::BnScreenCaptureListener { public: - binder::Status onScreenCaptureComplete(const ScreenCaptureResults& captureResults) override { + binder::Status onScreenCaptureCompleted(const ScreenCaptureResults& captureResults) override { resultsPromise.set_value(captureResults); return binder::Status::ok(); } ScreenCaptureResults waitForResults() { std::future resultsFuture = resultsPromise.get_future(); - return resultsFuture.get(); + const auto screenCaptureResults = resultsFuture.get(); + screenCaptureResults.fence->waitForever(""); + return screenCaptureResults; } private: diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 4225e92c60..27df232472 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5851,23 +5851,18 @@ status_t SurfaceFlinger::captureScreenCommon(RenderAreaFuture renderAreaFuture, if (!renderArea) { ALOGW("Skipping screen capture because of invalid render area."); captureResults.result = NO_MEMORY; - captureListener->onScreenCaptureComplete(captureResults); + captureListener->onScreenCaptureCompleted(captureResults); return; } status_t result = NO_ERROR; - int syncFd = -1; renderArea->render([&] { - result = renderScreenImplLocked(*renderArea, traverseLayers, buffer, forSystem, &syncFd, + result = renderScreenImplLocked(*renderArea, traverseLayers, buffer, forSystem, regionSampling, captureResults); }); - if (result == NO_ERROR) { - sync_wait(syncFd, -1); - close(syncFd); - } captureResults.result = result; - captureListener->onScreenCaptureComplete(captureResults); + captureListener->onScreenCaptureCompleted(captureResults); })); return NO_ERROR; @@ -5876,7 +5871,7 @@ status_t SurfaceFlinger::captureScreenCommon(RenderAreaFuture renderAreaFuture, status_t SurfaceFlinger::renderScreenImplLocked(const RenderArea& renderArea, TraverseLayersFunction traverseLayers, const sp& buffer, bool forSystem, - int* outSyncFd, bool regionSampling, + bool regionSampling, ScreenCaptureResults& captureResults) { ATRACE_CALL(); @@ -5985,14 +5980,14 @@ status_t SurfaceFlinger::renderScreenImplLocked(const RenderArea& renderArea, getRenderEngine().drawLayers(clientCompositionDisplay, clientCompositionLayerPointers, buffer, /*useFramebufferCache=*/false, std::move(bufferFence), &drawFence); - *outSyncFd = drawFence.release(); - - if (*outSyncFd >= 0) { - sp releaseFence = new Fence(dup(*outSyncFd)); + if (drawFence >= 0) { + sp releaseFence = new Fence(dup(drawFence)); for (auto* layer : renderedLayers) { layer->onLayerDisplayed(releaseFence); } } + + captureResults.fence = new Fence(drawFence.release()); // Always switch back to unprotected context. getRenderEngine().useProtectedContext(false); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 1deef6eb17..50d6099698 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -815,8 +815,8 @@ class SurfaceFlinger : public BnSurfaceComposer, status_t captureScreenCommon(RenderAreaFuture, TraverseLayersFunction, sp&, bool regionSampling, const sp&); status_t renderScreenImplLocked(const RenderArea&, TraverseLayersFunction, - const sp&, bool forSystem, int* outSyncFd, - bool regionSampling, ScreenCaptureResults&); + const sp&, bool forSystem, bool regionSampling, + ScreenCaptureResults&); sp getDisplayByIdOrLayerStack(uint64_t displayOrLayerStack) REQUIRES(mStateLock); sp getDisplayByLayerStack(uint64_t layerStack) REQUIRES(mStateLock); diff --git a/services/surfaceflinger/tests/LayerState_test.cpp b/services/surfaceflinger/tests/LayerState_test.cpp index ecfed13adb..93d5f2f8ec 100644 --- a/services/surfaceflinger/tests/LayerState_test.cpp +++ b/services/surfaceflinger/tests/LayerState_test.cpp @@ -83,6 +83,7 @@ TEST(LayerStateTest, ParcellingLayerCaptureArgs) { TEST(LayerStateTest, ParcellingScreenCaptureResults) { ScreenCaptureResults results; results.buffer = new GraphicBuffer(100, 200, PIXEL_FORMAT_RGBA_8888, 1, 0); + results.fence = new Fence(dup(fileno(tmpfile()))); results.capturedSecureLayers = true; results.capturedDataspace = ui::Dataspace::DISPLAY_P3; results.result = BAD_VALUE; @@ -99,6 +100,7 @@ TEST(LayerStateTest, ParcellingScreenCaptureResults) { ASSERT_EQ(results.buffer->getWidth(), results2.buffer->getWidth()); ASSERT_EQ(results.buffer->getHeight(), results2.buffer->getHeight()); ASSERT_EQ(results.buffer->getPixelFormat(), results2.buffer->getPixelFormat()); + ASSERT_EQ(results.fence->isValid(), results2.fence->isValid()); ASSERT_EQ(results.capturedSecureLayers, results2.capturedSecureLayers); ASSERT_EQ(results.capturedDataspace, results2.capturedDataspace); ASSERT_EQ(results.result, results2.result); diff --git a/services/surfaceflinger/tests/unittests/CompositionTest.cpp b/services/surfaceflinger/tests/unittests/CompositionTest.cpp index 83e3ba414e..f2051d91c9 100644 --- a/services/surfaceflinger/tests/unittests/CompositionTest.cpp +++ b/services/surfaceflinger/tests/unittests/CompositionTest.cpp @@ -253,14 +253,9 @@ void CompositionTest::captureScreenComposition() { mCaptureScreenBuffer = new GraphicBuffer(renderArea->getReqWidth(), renderArea->getReqHeight(), HAL_PIXEL_FORMAT_RGBA_8888, 1, usage, "screenshot"); - int fd = -1; status_t result = mFlinger.renderScreenImplLocked(*renderArea, traverseLayers, mCaptureScreenBuffer.get(), - forSystem, &fd, regionSampling); - if (fd >= 0) { - close(fd); - } - + forSystem, regionSampling); EXPECT_EQ(NO_ERROR, result); LayerCase::cleanup(this); diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index fc284ff948..2701f472aa 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -349,11 +349,11 @@ class TestableSurfaceFlinger final : private ISchedulerCallback { auto renderScreenImplLocked(const RenderArea& renderArea, SurfaceFlinger::TraverseLayersFunction traverseLayers, - const sp& buffer, bool forSystem, int* outSyncFd, + const sp& buffer, bool forSystem, bool regionSampling) { ScreenCaptureResults captureResults; return mFlinger->renderScreenImplLocked(renderArea, traverseLayers, buffer, forSystem, - outSyncFd, regionSampling, captureResults); + regionSampling, captureResults); } auto traverseLayersInLayerStack(ui::LayerStack layerStack, int32_t uid,