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
pa-faiz · Jan 29, 2021 · 9959994 · 9959994
1 parent 31999ef
commit 9959994
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 26 deletions.
diff --git 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<uint32_t>(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
@@ -20,5 +20,5 @@ import android.gui.ScreenCaptureResults;
 
 /** @hide */
 oneway interface IScreenCaptureListener {
-    void onScreenCaptureComplete(in ScreenCaptureResults captureResults);
+    void onScreenCaptureCompleted(in ScreenCaptureResults captureResults);
 }
diff --git a/libs/gui/include/gui/ScreenCaptureResults.h b/libs/gui/include/gui/ScreenCaptureResults.h
@@ -27,6 +27,7 @@
 
 #include <binder/Parcel.h>
 #include <binder/Parcelable.h>
+#include <ui/Fence.h>
 #include <ui/GraphicBuffer.h>
 
 namespace android::gui {
@@ -39,6 +40,7 @@ struct ScreenCaptureResults : public Parcelable {
     status_t readFromParcel(const android::Parcel* parcel) override;
 
     sp<GraphicBuffer> buffer;
+    sp<Fence> 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
@@ -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<ScreenCaptureResults> 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
@@ -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<GraphicBuffer>& 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<Fence> releaseFence = new Fence(dup(*outSyncFd));
+    if (drawFence >= 0) {
+        sp<Fence> 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
@@ -815,8 +815,8 @@ class SurfaceFlinger : public BnSurfaceComposer,
     status_t captureScreenCommon(RenderAreaFuture, TraverseLayersFunction, sp<GraphicBuffer>&,
                                  bool regionSampling, const sp<IScreenCaptureListener>&);
     status_t renderScreenImplLocked(const RenderArea&, TraverseLayersFunction,
-                                    const sp<GraphicBuffer>&, bool forSystem, int* outSyncFd,
-                                    bool regionSampling, ScreenCaptureResults&);
+                                    const sp<GraphicBuffer>&, bool forSystem, bool regionSampling,
+                                    ScreenCaptureResults&);
 
     sp<DisplayDevice> getDisplayByIdOrLayerStack(uint64_t displayOrLayerStack) REQUIRES(mStateLock);
     sp<DisplayDevice> getDisplayByLayerStack(uint64_t layerStack) REQUIRES(mStateLock);

diff --git 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
@@ -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
@@ -349,11 +349,11 @@ class TestableSurfaceFlinger final : private ISchedulerCallback {
 
     auto renderScreenImplLocked(const RenderArea& renderArea,
                                 SurfaceFlinger::TraverseLayersFunction traverseLayers,
-                                const sp<GraphicBuffer>& buffer, bool forSystem, int* outSyncFd,
+                                const sp<GraphicBuffer>& 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,