Use FenceResult in ScreenCaptureResults

Bug: b/232535621
Test: atest SurfaceFlinger_test
Change-Id: I9295202cb2e72e9b078815b24468b588a89b6899

8 files changed, 59 insertions, 23 deletions

diff --git a/libs/gui/ScreenCaptureResults.cpp b/libs/gui/ScreenCaptureResults.cpp
index fe387064bc..601a5f9b33 100644
--- a/libs/gui/ScreenCaptureResults.cpp
+++ b/libs/gui/ScreenCaptureResults.cpp
@@ -17,6 +17,7 @@
 #include <gui/ScreenCaptureResults.h>
 
 #include <private/gui/ParcelUtils.h>
+#include <ui/FenceResult.h>
 
 namespace android::gui {
 
@@ -28,17 +29,17 @@ status_t ScreenCaptureResults::writeToParcel(android::Parcel* parcel) const {
         SAFE_PARCEL(parcel->writeBool, false);
     }
 
-    if (fence != Fence::NO_FENCE) {
+    if (fenceResult.ok() && fenceResult.value() != Fence::NO_FENCE) {
         SAFE_PARCEL(parcel->writeBool, true);
-        SAFE_PARCEL(parcel->write, *fence);
+        SAFE_PARCEL(parcel->write, *fenceResult.value());
     } else {
         SAFE_PARCEL(parcel->writeBool, false);
+        SAFE_PARCEL(parcel->writeInt32, fenceStatus(fenceResult));
     }
 
     SAFE_PARCEL(parcel->writeBool, capturedSecureLayers);
     SAFE_PARCEL(parcel->writeBool, capturedHdrLayers);
     SAFE_PARCEL(parcel->writeUint32, static_cast<uint32_t>(capturedDataspace));
-    SAFE_PARCEL(parcel->writeInt32, result);
     return NO_ERROR;
 }
 
@@ -53,8 +54,13 @@ status_t ScreenCaptureResults::readFromParcel(const android::Parcel* parcel) {
     bool hasFence;
     SAFE_PARCEL(parcel->readBool, &hasFence);
     if (hasFence) {
-        fence = new Fence();
-        SAFE_PARCEL(parcel->read, *fence);
+        fenceResult = sp<Fence>::make();
+        SAFE_PARCEL(parcel->read, *fenceResult.value());
+    } else {
+        status_t status;
+        SAFE_PARCEL(parcel->readInt32, &status);
+        fenceResult = status == NO_ERROR ? FenceResult(Fence::NO_FENCE)
+                                         : FenceResult(base::unexpected(status));
     }
 
     SAFE_PARCEL(parcel->readBool, &capturedSecureLayers);
@@ -62,7 +68,6 @@ status_t ScreenCaptureResults::readFromParcel(const android::Parcel* parcel) {
     uint32_t dataspace = 0;
     SAFE_PARCEL(parcel->readUint32, &dataspace);
     capturedDataspace = static_cast<ui::Dataspace>(dataspace);
-    SAFE_PARCEL(parcel->readInt32, &result);
     return NO_ERROR;
 }
 
diff --git a/libs/gui/include/gui/ScreenCaptureResults.h b/libs/gui/include/gui/ScreenCaptureResults.h
index 724c11c881..6e17791a29 100644
--- a/libs/gui/include/gui/ScreenCaptureResults.h
+++ b/libs/gui/include/gui/ScreenCaptureResults.h
@@ -19,6 +19,7 @@
 #include <binder/Parcel.h>
 #include <binder/Parcelable.h>
 #include <ui/Fence.h>
+#include <ui/FenceResult.h>
 #include <ui/GraphicBuffer.h>
 
 namespace android::gui {
@@ -31,11 +32,10 @@ public:
     status_t readFromParcel(const android::Parcel* parcel) override;
 
     sp<GraphicBuffer> buffer;
-    sp<Fence> fence = Fence::NO_FENCE;
+    FenceResult fenceResult = Fence::NO_FENCE;
     bool capturedSecureLayers{false};
     bool capturedHdrLayers{false};
     ui::Dataspace capturedDataspace{ui::Dataspace::V0_SRGB};
-    status_t result = OK;
 };
 
 } // namespace android::gui
diff --git a/libs/gui/include/gui/SyncScreenCaptureListener.h b/libs/gui/include/gui/SyncScreenCaptureListener.h
index 0784fbc058..bcf565a494 100644
--- a/libs/gui/include/gui/SyncScreenCaptureListener.h
+++ b/libs/gui/include/gui/SyncScreenCaptureListener.h
@@ -34,7 +34,9 @@ public:
     ScreenCaptureResults waitForResults() {
         std::future<ScreenCaptureResults> resultsFuture = resultsPromise.get_future();
         const auto screenCaptureResults = resultsFuture.get();
-        screenCaptureResults.fence->waitForever("");
+        if (screenCaptureResults.fenceResult.ok()) {
+            screenCaptureResults.fenceResult.value()->waitForever("");
+        }
         return screenCaptureResults;
     }
 
@@ -42,4 +44,4 @@ private:
     std::promise<ScreenCaptureResults> resultsPromise;
 };
 
-} // namespace android
\ No newline at end of file
+} // namespace android
diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp
index 3e563b2aa2..c4c2fa5b85 100644
--- a/libs/gui/tests/BLASTBufferQueue_test.cpp
+++ b/libs/gui/tests/BLASTBufferQueue_test.cpp
@@ -311,7 +311,7 @@ protected:
             return err;
         }
         captureResults = captureListener->waitForResults();
-        return captureResults.result;
+        return fenceStatus(captureResults.fenceResult);
     }
 
     void queueBuffer(sp<IGraphicBufferProducer> igbp, uint8_t r, uint8_t g, uint8_t b,
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index 71f2ad4d3c..b9358e7717 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -218,7 +218,7 @@ protected:
             return err;
         }
         captureResults = captureListener->waitForResults();
-        return captureResults.result;
+        return fenceStatus(captureResults.fenceResult);
     }
 
     sp<Surface> mSurface;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index bdc8406575..b06d951bb6 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -6439,7 +6439,7 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenCommon(
         std::unique_ptr<RenderArea> renderArea = renderAreaFuture.get();
         if (!renderArea) {
             ALOGW("Skipping screen capture because of invalid render area.");
-            captureResults.result = NO_MEMORY;
+            captureResults.fenceResult = base::unexpected(NO_MEMORY);
             captureListener->onScreenCaptureCompleted(captureResults);
             return ftl::yield<FenceResult>(base::unexpected(NO_ERROR)).share();
         }
@@ -6456,9 +6456,7 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenCommon(
             return ftl::Future(std::move(renderFuture))
                     .then([captureListener, captureResults = std::move(captureResults)](
                                   FenceResult fenceResult) mutable -> FenceResult {
-                        // TODO(b/232535621): Change ScreenCaptureResults to store a FenceResult.
-                        captureResults.result = fenceStatus(fenceResult);
-                        captureResults.fence = std::move(fenceResult).value_or(Fence::NO_FENCE);
+                        captureResults.fenceResult = std::move(fenceResult);
                         captureListener->onScreenCaptureCompleted(captureResults);
                         return base::unexpected(NO_ERROR);
                     })
diff --git a/services/surfaceflinger/tests/LayerState_test.cpp b/services/surfaceflinger/tests/LayerState_test.cpp
index fbbf322732..21813700a2 100644
--- a/services/surfaceflinger/tests/LayerState_test.cpp
+++ b/services/surfaceflinger/tests/LayerState_test.cpp
@@ -90,13 +90,12 @@ TEST(LayerStateTest, ParcellingLayerCaptureArgs) {
     ASSERT_EQ(args.grayscale, args2.grayscale);
 }
 
-TEST(LayerStateTest, ParcellingScreenCaptureResults) {
+TEST(LayerStateTest, ParcellingScreenCaptureResultsWithFence) {
     ScreenCaptureResults results;
     results.buffer = sp<GraphicBuffer>::make(100u, 200u, PIXEL_FORMAT_RGBA_8888, 1u, 0u);
-    results.fence = sp<Fence>::make(dup(fileno(tmpfile())));
+    results.fenceResult = sp<Fence>::make(dup(fileno(tmpfile())));
     results.capturedSecureLayers = true;
     results.capturedDataspace = ui::Dataspace::DISPLAY_P3;
-    results.result = BAD_VALUE;
 
     Parcel p;
     results.writeToParcel(&p);
@@ -110,10 +109,41 @@ 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_TRUE(results.fenceResult.ok());
+    ASSERT_TRUE(results2.fenceResult.ok());
+    ASSERT_EQ(results.fenceResult.value()->isValid(), results2.fenceResult.value()->isValid());
     ASSERT_EQ(results.capturedSecureLayers, results2.capturedSecureLayers);
     ASSERT_EQ(results.capturedDataspace, results2.capturedDataspace);
-    ASSERT_EQ(results.result, results2.result);
+}
+
+TEST(LayerStateTest, ParcellingScreenCaptureResultsWithNoFenceOrError) {
+    ScreenCaptureResults results;
+
+    Parcel p;
+    results.writeToParcel(&p);
+    p.setDataPosition(0);
+
+    ScreenCaptureResults results2;
+    results2.readFromParcel(&p);
+
+    ASSERT_TRUE(results2.fenceResult.ok());
+    ASSERT_EQ(results2.fenceResult.value(), Fence::NO_FENCE);
+}
+
+TEST(LayerStateTest, ParcellingScreenCaptureResultsWithFenceError) {
+    ScreenCaptureResults results;
+    results.fenceResult = base::unexpected(BAD_VALUE);
+
+    Parcel p;
+    results.writeToParcel(&p);
+    p.setDataPosition(0);
+
+    ScreenCaptureResults results2;
+    results2.readFromParcel(&p);
+
+    ASSERT_FALSE(results.fenceResult.ok());
+    ASSERT_FALSE(results2.fenceResult.ok());
+    ASSERT_EQ(results.fenceResult.error(), results2.fenceResult.error());
 }
 
 } // namespace test
diff --git a/services/surfaceflinger/tests/utils/ScreenshotUtils.h b/services/surfaceflinger/tests/utils/ScreenshotUtils.h
index cb7040a68b..224868c014 100644
--- a/services/surfaceflinger/tests/utils/ScreenshotUtils.h
+++ b/services/surfaceflinger/tests/utils/ScreenshotUtils.h
@@ -18,6 +18,7 @@
 #include <gui/AidlStatusUtil.h>
 #include <gui/SyncScreenCaptureListener.h>
 #include <private/gui/ComposerServiceAIDL.h>
+#include <ui/FenceResult.h>
 #include <ui/Rect.h>
 #include <utils/String8.h>
 #include <functional>
@@ -46,7 +47,7 @@ public:
             return err;
         }
         captureResults = captureListener->waitForResults();
-        return captureResults.result;
+        return fenceStatus(captureResults.fenceResult);
     }
 
     static void captureScreen(std::unique_ptr<ScreenCapture>* sc) {
@@ -80,7 +81,7 @@ public:
             return err;
         }
         captureResults = captureListener->waitForResults();
-        return captureResults.result;
+        return fenceStatus(captureResults.fenceResult);
     }
 
     static void captureLayers(std::unique_ptr<ScreenCapture>* sc, LayerCaptureArgs& captureArgs) {