From e6c23e38d3afc3d06edfbddcabca26eceafe34b8 Mon Sep 17 00:00:00 2001
From: Michael Ludwig <[email protected]>
Date: Tue, 26 May 2026 13:26:32 -0400
Subject: [PATCH] [ganesh] Use submitted proc to confirm async reads were issued

Ganesh's finish proc doesn't have a status, but if flushSurface()
failed for unrelated reasons (due to queued operations before the
asyncRead was requested), the finish proc would be executed without
actually submitting the copy command to the GPU.

This tracks whether or not it was successfully submitted and makes
that a requirement before handing the result to the original callback.
The code is updated to more closely match Graphite's structure,
including unmapping any buffers on failure.

Bug: 513340227
Change-Id: If2998749a754d114b70e26e312a9a4cb08ea20e2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1242617
Commit-Queue: Michael Ludwig <[email protected]>
Reviewed-by: Greg Daniel <[email protected]>
---

diff --git a/src/gpu/ganesh/SurfaceContext.cpp b/src/gpu/ganesh/SurfaceContext.cpp
index c6fa47f..1afa768 100644
--- a/src/gpu/ganesh/SurfaceContext.cpp
+++ b/src/gpu/ganesh/SurfaceContext.cpp
@@ -670,6 +670,114 @@
                                    callbackContext);
 }
 
+// Shared between RGBA and YUVA readbacks.
+//
+// It is used as a finish proc and a submitted proc to confirm that the commands to write to the
+// buffer were submitted to the GPU. The finish proc then completes the process by notifying the
+// async result callback. This class manages logic to trigger the client callback regardless of the
+// order Ganesh runs the finish or submit procs and handles failure modes.
+struct SurfaceContext::AsyncReadPixelContext {
+    ReadPixelsCallback* fClientCallback;
+    ReadPixelsContext fClientContext;
+    SkISize fSize;
+    GrClientMappedBufferManager* fMappedBufferManager;
+
+    PixelTransferResult fPrimaryTransfer; // RGBA or Y
+    PixelTransferResult fUTransfer = {};
+    PixelTransferResult fVTransfer = {};
+    PixelTransferResult fATransfer = {};
+
+    AsyncReadPixelContext(ReadPixelsCallback* clientCallback,
+                          ReadPixelsContext clientContext,
+                          SkISize size,
+                          GrClientMappedBufferManager* manager,
+                          PixelTransferResult&& rgbaTransfer)
+            : fClientCallback(clientCallback)
+            , fClientContext(clientContext)
+            , fSize(size)
+            , fMappedBufferManager(manager)
+            , fPrimaryTransfer(std::move(rgbaTransfer))
+            , fUTransfer{}
+            , fVTransfer{}
+            , fATransfer{} {}
+
+    AsyncReadPixelContext(ReadPixelsCallback* clientCallback,
+                          ReadPixelsContext clientContext,
+                          SkISize size,
+                          GrClientMappedBufferManager* manager,
+                          PixelTransferResult&& yTransfer,
+                          PixelTransferResult&& uTransfer,
+                          PixelTransferResult&& vTransfer,
+                          PixelTransferResult&& aTransfer)
+            : fClientCallback(clientCallback)
+            , fClientContext(clientContext)
+            , fSize(size)
+            , fMappedBufferManager(manager)
+            , fPrimaryTransfer(std::move(yTransfer))
+            , fUTransfer(std::move(uTransfer))
+            , fVTransfer(std::move(vTransfer))
+            , fATransfer(std::move(aTransfer)) {}
+
+    void setSubmitted(bool success) {
+        SkASSERT(fSubmitted == kUnsubmitted);
+        fSubmitted = success ? kSuccess : kFailure;
+        this->runClientCallbackMaybe();
+    }
+
+    void setFinished() {
+        fFinished = true;
+        this->runClientCallbackMaybe();
+    }
+
+    // This will destroy itself once it runs
+    void runClientCallbackMaybe() {
+        using AsyncReadResult = skgpu::TAsyncReadResult<GrGpuBuffer,
+                                                        GrDirectContext::DirectContextID,
+                                                        PixelTransferResult>;
+
+        if (!fFinished || fSubmitted == kUnsubmitted) {
+            return; // wait for both finish and submit procs to trigger
+        }
+
+        // Once both procs have fired, then proceed with the client callback.
+        std::unique_ptr<AsyncReadResult> result;
+        if (fSubmitted == kSuccess) {
+            result = std::make_unique<AsyncReadResult>(fMappedBufferManager->ownerID());
+        } // else the submit failed so we need to invoke the client callback with null
+
+
+        using Plane = std::pair<const PixelTransferResult*, SkISize>;
+        SkISize uvSize = {fSize.width() / 2, fSize.height() / 2};
+        for (auto [transfer, size] : {Plane{&fPrimaryTransfer, fSize},
+                                      Plane{&fUTransfer, uvSize},
+                                      Plane{&fVTransfer, uvSize},
+                                      Plane{&fATransfer, fSize}}) {
+            if (!transfer->fTransferBuffer) {
+                // We reach this for RGBA transfers (just the first plane), or for YUV w/o an alpha.
+                break;
+            }
+            if (result && !result->addTransferResult(*transfer,
+                                                     size,
+                                                     transfer->fRowBytes,
+                                                     fMappedBufferManager)) {
+                result.reset();
+            }
+            if (!result && transfer->fTransferBuffer->isMapped()) {
+                transfer->fTransferBuffer->unmap();
+            }
+        }
+        (*fClientCallback)(fClientContext, std::move(result));
+
+        delete this;
+    }
+
+private:
+    enum AsyncSubmitStatus { kUnsubmitted, kSuccess, kFailure };
+
+    AsyncSubmitStatus fSubmitted = kUnsubmitted;
+    bool fFinished = false;
+};
+
 void SurfaceContext::asyncReadPixels(GrDirectContext* dContext,
                                      const SkIRect& rect,
                                      SkColorType colorType,
@@ -707,37 +815,23 @@
         return;
     }
 
-    struct FinishContext {
-        ReadPixelsCallback* fClientCallback;
-        ReadPixelsContext fClientContext;
-        SkISize fSize;
-        GrClientMappedBufferManager* fMappedBufferManager;
-        PixelTransferResult fTransferResult;
-    };
     // Assumption is that the caller would like to flush. We could take a parameter or require an
     // explicit flush from the caller. We'd have to have a way to defer attaching the finish
     // callback to GrGpu until after the next flush that flushes our op list, though.
-    auto* finishContext = new FinishContext{callback,
-                                            callbackContext,
-                                            rect.size(),
-                                            mappedBufferManager,
-                                            std::move(transferResult)};
-    auto finishCallback = [](GrGpuFinishedContext c) {
-        const auto* context = reinterpret_cast<const FinishContext*>(c);
-        auto manager = context->fMappedBufferManager;
-        auto result = std::make_unique<AsyncReadResult>(manager->ownerID());
-        if (!result->addTransferResult(context->fTransferResult,
-                                       context->fSize,
-                                       context->fTransferResult.fRowBytes,
-                                       manager)) {
-            result.reset();
-        }
-        (*context->fClientCallback)(context->fClientContext, std::move(result));
-        delete context;
-    };
+    auto* asyncContext = new AsyncReadPixelContext{callback,
+                                                   callbackContext,
+                                                   rect.size(),
+                                                   mappedBufferManager,
+                                                   std::move(transferResult)};
+
     GrFlushInfo flushInfo;
-    flushInfo.fFinishedContext = finishContext;
-    flushInfo.fFinishedProc = finishCallback;
+    flushInfo.fSubmittedContext = flushInfo.fFinishedContext = asyncContext;
+    flushInfo.fSubmittedProc = [](GrGpuSubmittedContext c, bool success) {
+        reinterpret_cast<AsyncReadPixelContext*>(c)->setSubmitted(success);
+    };
+    flushInfo.fFinishedProc = [](GrGpuSubmittedContext c) {
+        reinterpret_cast<AsyncReadPixelContext*>(c)->setFinished();
+    };
 
     dContext->priv().flushSurface(
             this->asSurfaceProxy(), SkSurfaces::BackendSurfaceAccess::kNoAccess, flushInfo);
@@ -977,71 +1071,27 @@
         return;
     }
 
-    struct FinishContext {
-        ReadPixelsCallback* fClientCallback;
-        ReadPixelsContext fClientContext;
-        GrClientMappedBufferManager* fMappedBufferManager;