From defc3a5a92966c32cb2a6a901e2fa3036a13bb8a Mon Sep 17 00:00:00 2001
From: Nicolette Prevost <nicolettep@google.com>
Date: Wed, 20 May 2026 12:05:45 -0400
Subject: [PATCH] Report and handle failure for inlineUpload(...) calls

* The bug associated with this change called out that we should not attempt to perform an upload on an externally-owned secondary command buffer.

* Exiting early does not appropriately signal that an upload failed, so modify `GrOpsRenderPass::inlineUpload(...)` base class to return a bool indicating success or failure. Upon failure, do not attempt the associated immediate draw call and report that the draw failed.

* To maintain current behavior, have `inlineUpload(...)` return true in nearly all cases except that identified in the associated bug.

Bug: https://issues.chromium.org/issues/513948178
Change-Id: Idfeb2527062818e5fa63a38112dd1c26f7847998
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1239496
Commit-Queue: Nicolette Prevost <nicolettep@google.com>
Auto-Submit: Nicolette Prevost <nicolettep@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
---
 src/gpu/ganesh/GrOpFlushState.cpp         | 42 +++++++++++++++--------
 src/gpu/ganesh/GrOpsRenderPass.h          |  2 +-
 src/gpu/ganesh/d3d/GrD3DOpsRenderPass.cpp |  5 +--
 src/gpu/ganesh/d3d/GrD3DOpsRenderPass.h   |  2 +-
 src/gpu/ganesh/gl/GrGLOpsRenderPass.h     |  3 +-
 src/gpu/ganesh/mock/GrMockOpsRenderPass.h |  2 +-
 src/gpu/ganesh/mtl/GrMtlOpsRenderPass.h   |  2 +-
 src/gpu/ganesh/mtl/GrMtlOpsRenderPass.mm  |  4 ++-
 src/gpu/ganesh/vk/GrVkOpsRenderPass.cpp   | 15 ++++++--
 src/gpu/ganesh/vk/GrVkOpsRenderPass.h     |  3 +-
 10 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/src/gpu/ganesh/GrOpFlushState.cpp b/src/gpu/ganesh/GrOpFlushState.cpp
index 27768cd115..68cb7d55d5 100644
--- a/src/gpu/ganesh/GrOpFlushState.cpp
+++ b/src/gpu/ganesh/GrOpFlushState.cpp
@@ -53,27 +53,39 @@ void GrOpFlushState::executeDrawsAndUploadsForMeshDrawOp(
 
     while (fCurrDraw != fDraws.end() && fCurrDraw->fOp == op) {
         skgpu::Token drawToken = fTokenTracker->nextFlushToken();
+
+        bool drawUploadFailure = false;
         while (fCurrUpload != fInlineUploads.end() &&
                fCurrUpload->fUploadBeforeToken == drawToken) {
-            this->opsRenderPass()->inlineUpload(this, fCurrUpload->fUpload);
+            if (!this->opsRenderPass()->inlineUpload(this, fCurrUpload->fUpload)) {
+                drawUploadFailure = true;
+            }
+            // Attempt subsequent uploads even if one fails (future draws may depend upon them).
             ++fCurrUpload;
         }
 
-        GrProgramInfo programInfo(this->caps(),
-                                  this->writeView(),
-                                  this->usesMSAASurface(),
-                                  pipeline,
-                                  userStencilSettings,
-                                  fCurrDraw->fGeometryProcessor,
-                                  fCurrDraw->fPrimitiveType,
-                                  this->renderPassBarriers(),
-                                  this->colorLoadOp());
+        // If not all of the uploads succeeded, do not attempt to execute the draw and relevant
+        // preparations. Continue on to the next draw operation (which may also fail if subsequent
+        // draw calls depended upon these uploads, but we cannot know that at this point).
+        if (drawUploadFailure) {
+            fGpu->stats()->incNumFailedDraws();
+        } else {
+            GrProgramInfo programInfo(this->caps(),
+                                      this->writeView(),
+                                      this->usesMSAASurface(),
+                                      pipeline,
+                                      userStencilSettings,
+                                      fCurrDraw->fGeometryProcessor,
+                                      fCurrDraw->fPrimitiveType,
+                                      this->renderPassBarriers(),
+                                      this->colorLoadOp());
 
-        this->bindPipelineAndScissorClip(programInfo, chainBounds);
-        this->bindTextures(programInfo.geomProc(), fCurrDraw->fGeomProcProxies,
-                           programInfo.pipeline());
-        for (int i = 0; i < fCurrDraw->fMeshCnt; ++i) {
-            this->drawMesh(fCurrDraw->fMeshes[i]);
+            this->bindPipelineAndScissorClip(programInfo, chainBounds);
+            this->bindTextures(programInfo.geomProc(), fCurrDraw->fGeomProcProxies,
+                            programInfo.pipeline());
+            for (int i = 0; i < fCurrDraw->fMeshCnt; ++i) {
+                this->drawMesh(fCurrDraw->fMeshes[i]);
+            }
         }
 
         fTokenTracker->issueFlushToken();
diff --git a/src/gpu/ganesh/GrOpsRenderPass.h b/src/gpu/ganesh/GrOpsRenderPass.h
index 656e906714..516931ffe5 100644
--- a/src/gpu/ganesh/GrOpsRenderPass.h
+++ b/src/gpu/ganesh/GrOpsRenderPass.h
@@ -129,7 +129,7 @@ public:
                           int baseVertex);
 
     // Performs an upload of vertex data in the middle of a set of a set of draws
-    virtual void inlineUpload(GrOpFlushState*, GrDeferredTextureUploadFn&) = 0;
+    virtual bool inlineUpload(GrOpFlushState*, GrDeferredTextureUploadFn&) = 0;
 
     /**
      * Clear the owned render target. Clears the full target if 'scissor' is disabled, otherwise it
diff --git a/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.cpp b/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.cpp
index cbf13875d0..5fcea0598d 100644
--- a/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.cpp
+++ b/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.cpp
@@ -339,16 +339,17 @@ void GrD3DOpsRenderPass::onClearStencilClip(const GrScissorState& scissor, bool
     fGpu->currentCommandList()->clearDepthStencilView(d3dStencil, stencilColor, &clearRect);
 }
 
-void GrD3DOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) {
+bool GrD3DOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) {
     // If we ever start using copy command lists for doing uploads, then we'll need to make sure
     // we submit our main command list before doing the copy here and then start a new main command
     // list.
-
     fGpu->endRenderPass(fRenderTarget, fOrigin, fBounds);
 
     // We pass in true here to signal that after the upload we need to set the upload texture's
     // resource state back to D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE.
     state->doUpload(upload, true);
+
+    return true;
 }
 
 void GrD3DOpsRenderPass::submit() {
diff --git a/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.h b/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.h
index 5da3d754e2..65db27bb78 100644
--- a/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.h
+++ b/src/gpu/ganesh/d3d/GrD3DOpsRenderPass.h
@@ -22,7 +22,7 @@ public:
 
     ~GrD3DOpsRenderPass() override;
 
-    void inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override;
+    bool inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override;
 
     void onExecuteDrawable(std::unique_ptr<SkDrawable::GpuDrawHandler>) override {}
 
diff --git a/src/gpu/ganesh/gl/GrGLOpsRenderPass.h b/src/gpu/ganesh/gl/GrGLOpsRenderPass.h
index c1367130e9..11642e5205 100644
--- a/src/gpu/ganesh/gl/GrGLOpsRenderPass.h
+++ b/src/gpu/ganesh/gl/GrGLOpsRenderPass.h
@@ -42,8 +42,9 @@ class GrGLOpsRenderPass : public GrOpsRenderPass {
 public:
     GrGLOpsRenderPass(GrGLGpu* gpu) : fGpu(gpu) {}
 
-    void inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override {
+    bool inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override {
         state->doUpload(upload);
+        return true;
     }
 
     void set(GrRenderTarget*, bool useMSAASurface, const SkIRect& contentBounds, GrSurfaceOrigin,
diff --git a/src/gpu/ganesh/mock/GrMockOpsRenderPass.h b/src/gpu/ganesh/mock/GrMockOpsRenderPass.h
index c4ca146763..f40c581410 100644
--- a/src/gpu/ganesh/mock/GrMockOpsRenderPass.h
+++ b/src/gpu/ganesh/mock/GrMockOpsRenderPass.h
@@ -42,7 +42,7 @@ public:
     }
 
     GrGpu* gpu() override { return fGpu; }
-    void inlineUpload(GrOpFlushState*, GrDeferredTextureUploadFn&) override {}
+    bool inlineUpload(GrOpFlushState*, GrDeferredTextureUploadFn&) override { return true; }
 
     int numDraws() const { return fNumDraws; }
 
diff --git a/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.h b/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.h
index 4b27579879..73f5ef4864 100644
--- a/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.h
+++ b/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.h
@@ -31,7 +31,7 @@ public:
 
     void initRenderState(GrMtlRenderCommandEncoder*);
 
-    void inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override;
+    bool inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override;
     void submit();
 
 private:
diff --git a/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.mm b/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.mm
index 3bf4195694..2460fd577f 100644
--- a/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.mm
+++ b/src/gpu/ganesh/mtl/GrMtlOpsRenderPass.mm
@@ -186,7 +186,7 @@ void GrMtlOpsRenderPass::onClearStencilClip(const GrScissorState& scissor, bool
     }
 }
 
-void GrMtlOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) {
+bool GrMtlOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) {
     state->doUpload(upload);
 
     // If the previous renderCommandEncoder did a resolve without an MSAA store
@@ -197,6 +197,8 @@ void GrMtlOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUp
         // create a new encoder at this point, though maybe not necessary.
         this->setupRenderCommandEncoder(nullptr);
     }
+
+    return true;
 }
 
 void GrMtlOpsRenderPass::initRenderState(GrMtlRenderCommandEncoder* encoder) {
diff --git a/src/gpu/ganesh/vk/GrVkOpsRenderPass.cpp b/src/gpu/ganesh/vk/GrVkOpsRenderPass.cpp
index 16d85a9c51..5f9fddd296 100644
--- a/src/gpu/ganesh/vk/GrVkOpsRenderPass.cpp
+++ b/src/gpu/ganesh/vk/GrVkOpsRenderPass.cpp
@@ -616,11 +616,18 @@ void GrVkOpsRenderPass::addAdditionalRenderPass(bool mustUseSecondaryCommandBuff
     this->beginRenderPass(vkClearColor, loadFromResolve);
 }
 
-void GrVkOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) {
+bool GrVkOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) {
     if (!fCurrentRenderPass) {
         SkASSERT(fGpu->isDeviceLost());
-        return;
+        return false;
     }
+
+    // Skia should not manipulate a wrapped (externally-owned) secondary command buffer, which
+    // is necessary in order to perform the requested inline upload.
+    if (this->wrapsSecondaryCommandBuffer()) {
+        return false;
+    }
+
     if (fCurrentSecondaryCommandBuffer) {
         fCurrentSecondaryCommandBuffer->end(fGpu);
         fGpu->submitSecondaryCommandBuffer(std::move(fCurrentSecondaryCommandBuffer));
@@ -629,9 +636,11 @@ void GrVkOpsRenderPass::inlineUpload(GrOpFlushState* state, GrDeferredTextureUpl
 
     // We pass in true here to signal that after the upload we need to set the upload textures
     // layout back to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL.
-    state->doUpload(upload, true);
+    state->doUpload(upload, /*shouldPrepareSurfaceForSampling=*/true);
 
     this->addAdditionalRenderPass(false);
+
+    return true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/src/gpu/ganesh/vk/GrVkOpsRenderPass.h b/src/gpu/ganesh/vk/GrVkOpsRenderPass.h
index ad145a6fed..a74222a6ab 100644
--- a/src/gpu/ganesh/vk/GrVkOpsRenderPass.h
+++ b/src/gpu/ganesh/vk/GrVkOpsRenderPass.h
@@ -46,7 +46,8 @@ public:
 
     ~GrVkOpsRenderPass() override;
 
-    void inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override;
+    // Returns true upon successful upload and false upon failure.
+    bool inlineUpload(GrOpFlushState* state, GrDeferredTextureUploadFn& upload) override;
 
     void onExecuteDrawable(std::unique_ptr<SkDrawable::GpuDrawHandler>) override;
 
-- 
2.43.0

