Move vulkan external commandbuffer logic from RT to GrVkFramebufer.

Bug: skia:11809
Change-Id: I7fa364f7472be35e02b06fa2f3fbb6d61a427f65
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/396156
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
diff --git a/src/gpu/GrProxyProvider.cpp b/src/gpu/GrProxyProvider.cpp
index 08d0964..1b25b06 100644
--- a/src/gpu/GrProxyProvider.cpp
+++ b/src/gpu/GrProxyProvider.cpp
@@ -658,8 +658,8 @@
 
     GrColorType colorType = SkColorTypeToGrColorType(imageInfo.colorType());
 
-    if (!this->caps()->isFormatAsColorTypeRenderable(colorType, rt->backendFormat(),
-                                                     rt->numSamples())) {
+    if (!this->caps()->isFormatAsColorTypeRenderable(
+            colorType, GrBackendFormat::MakeVk(vkInfo.fFormat), /*sampleCount=*/1)) {
         return nullptr;
     }
 
diff --git a/src/gpu/vk/GrVkCommandBuffer.cpp b/src/gpu/vk/GrVkCommandBuffer.cpp
index d82b53c..7db871e 100644
--- a/src/gpu/vk/GrVkCommandBuffer.cpp
+++ b/src/gpu/vk/GrVkCommandBuffer.cpp
@@ -56,7 +56,7 @@
 
 void GrVkCommandBuffer::releaseResources() {
     TRACE_EVENT0("skia.gpu", TRACE_FUNC);
-    SkASSERT(!fIsActive);
+    SkASSERT(!fIsActive || this->isWrapped());
     for (int i = 0; i < fTrackedResources.count(); ++i) {
         fTrackedResources[i]->notifyFinishedWithWorkOnGpu();
     }
@@ -933,52 +933,52 @@
     if (err) {
         return nullptr;
     }
-    return new GrVkSecondaryCommandBuffer(cmdBuffer, false);
+    return new GrVkSecondaryCommandBuffer(cmdBuffer, /*externalRenderPass=*/nullptr);
 }
 
-GrVkSecondaryCommandBuffer* GrVkSecondaryCommandBuffer::Create(VkCommandBuffer cmdBuffer) {
-    return new GrVkSecondaryCommandBuffer(cmdBuffer, true);
+GrVkSecondaryCommandBuffer* GrVkSecondaryCommandBuffer::Create(
+        VkCommandBuffer cmdBuffer, const GrVkRenderPass* externalRenderPass) {
+    return new GrVkSecondaryCommandBuffer(cmdBuffer, externalRenderPass);
 }
 
 void GrVkSecondaryCommandBuffer::begin(GrVkGpu* gpu, const GrVkFramebuffer* framebuffer,
                                        const GrVkRenderPass* compatibleRenderPass) {
     SkASSERT(!fIsActive);
+    SkASSERT(!this->isWrapped());
     SkASSERT(compatibleRenderPass);
     fActiveRenderPass = compatibleRenderPass;
 
-    if (!this->isWrapped()) {
-        VkCommandBufferInheritanceInfo inheritanceInfo;
-        memset(&inheritanceInfo, 0, sizeof(VkCommandBufferInheritanceInfo));
-        inheritanceInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_INHERITANCE_INFO;
-        inheritanceInfo.pNext = nullptr;
-        inheritanceInfo.renderPass = fActiveRenderPass->vkRenderPass();
-        inheritanceInfo.subpass = 0; // Currently only using 1 subpass for each render pass
-        inheritanceInfo.framebuffer = framebuffer ? framebuffer->framebuffer() : VK_NULL_HANDLE;
-        inheritanceInfo.occlusionQueryEnable = false;
-        inheritanceInfo.queryFlags = 0;
-        inheritanceInfo.pipelineStatistics = 0;
+    VkCommandBufferInheritanceInfo inheritanceInfo;
+    memset(&inheritanceInfo, 0, sizeof(VkCommandBufferInheritanceInfo));
+    inheritanceInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_INHERITANCE_INFO;
+    inheritanceInfo.pNext = nullptr;
+    inheritanceInfo.renderPass = fActiveRenderPass->vkRenderPass();
+    inheritanceInfo.subpass = 0; // Currently only using 1 subpass for each render pass
+    inheritanceInfo.framebuffer = framebuffer ? framebuffer->framebuffer() : VK_NULL_HANDLE;
+    inheritanceInfo.occlusionQueryEnable = false;
+    inheritanceInfo.queryFlags = 0;
+    inheritanceInfo.pipelineStatistics = 0;
 
-        VkCommandBufferBeginInfo cmdBufferBeginInfo;
-        memset(&cmdBufferBeginInfo, 0, sizeof(VkCommandBufferBeginInfo));
-        cmdBufferBeginInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
-        cmdBufferBeginInfo.pNext = nullptr;
-        cmdBufferBeginInfo.flags = VK_COMMAND_BUFFER_USAGE_RENDER_PASS_CONTINUE_BIT |
-                VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
-        cmdBufferBeginInfo.pInheritanceInfo = &inheritanceInfo;
+    VkCommandBufferBeginInfo cmdBufferBeginInfo;
+    memset(&cmdBufferBeginInfo, 0, sizeof(VkCommandBufferBeginInfo));
+    cmdBufferBeginInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
+    cmdBufferBeginInfo.pNext = nullptr;
+    cmdBufferBeginInfo.flags = VK_COMMAND_BUFFER_USAGE_RENDER_PASS_CONTINUE_BIT |
+            VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
+    cmdBufferBeginInfo.pInheritanceInfo = &inheritanceInfo;
 
-        GR_VK_CALL_ERRCHECK(gpu, BeginCommandBuffer(fCmdBuffer, &cmdBufferBeginInfo));
-    }
+    GR_VK_CALL_ERRCHECK(gpu, BeginCommandBuffer(fCmdBuffer, &cmdBufferBeginInfo));
+
     fIsActive = true;
 }
 
 void GrVkSecondaryCommandBuffer::end(GrVkGpu* gpu) {
     SkASSERT(fIsActive);
-    if (!this->isWrapped()) {
-        GR_VK_CALL_ERRCHECK(gpu, EndCommandBuffer(fCmdBuffer));
-    }
+    SkASSERT(!this->isWrapped());
+    GR_VK_CALL_ERRCHECK(gpu, EndCommandBuffer(fCmdBuffer));
     this->invalidateState();
-    fIsActive = false;
     fHasWork = false;
+    fIsActive = false;
 }
 
 void GrVkSecondaryCommandBuffer::recycle(GrVkCommandPool* cmdPool) {
diff --git a/src/gpu/vk/GrVkCommandBuffer.h b/src/gpu/vk/GrVkCommandBuffer.h
index 7e501d6..8234063 100644
--- a/src/gpu/vk/GrVkCommandBuffer.h
+++ b/src/gpu/vk/GrVkCommandBuffer.h
@@ -145,7 +145,8 @@
 
 protected:
     GrVkCommandBuffer(VkCommandBuffer cmdBuffer, bool isWrapped = false)
-            : fCmdBuffer(cmdBuffer)
+            : fIsActive(isWrapped) // All wrapped command buffers start as active
+            , fCmdBuffer(cmdBuffer)
             , fIsWrapped(isWrapped) {
         this->invalidateState();
     }
@@ -168,7 +169,7 @@
 
     // Tracks whether we are in the middle of a command buffer begin/end calls and thus can add
     // new commands to the buffer;
-    bool                      fIsActive = false;
+    bool                      fIsActive;
     bool                      fHasWork = false;
 
     // Stores a pointer to the current active render pass (i.e. begin has been called but not
@@ -340,7 +341,8 @@
 public:
     static GrVkSecondaryCommandBuffer* Create(GrVkGpu* gpu, GrVkCommandPool* cmdPool);
     // Used for wrapping an external secondary command buffer.
-    static GrVkSecondaryCommandBuffer* Create(VkCommandBuffer externalSecondaryCB);
+    static GrVkSecondaryCommandBuffer* Create(VkCommandBuffer externalSecondaryCB,
+                                              const GrVkRenderPass* externalRenderPass);
 
     void begin(GrVkGpu* gpu, const GrVkFramebuffer* framebuffer,
                const GrVkRenderPass* compatibleRenderPass);
@@ -351,8 +353,11 @@
     VkCommandBuffer vkCommandBuffer() { return fCmdBuffer; }
 
 private:
-    explicit GrVkSecondaryCommandBuffer(VkCommandBuffer cmdBuffer, bool isWrapped)
-        : INHERITED(cmdBuffer, isWrapped) {}
+    explicit GrVkSecondaryCommandBuffer(VkCommandBuffer cmdBuffer,
+                                        const GrVkRenderPass* externalRenderPass)
+            : INHERITED(cmdBuffer, SkToBool(externalRenderPass)) {
+        fActiveRenderPass = externalRenderPass;
+    }
 
     void onFreeGPUData(const GrVkGpu* gpu) const override {}
 
diff --git a/src/gpu/vk/GrVkFramebuffer.cpp b/src/gpu/vk/GrVkFramebuffer.cpp
index e2170bc..ec14134 100644
--- a/src/gpu/vk/GrVkFramebuffer.cpp
+++ b/src/gpu/vk/GrVkFramebuffer.cpp
@@ -8,6 +8,7 @@
 #include "src/gpu/vk/GrVkFramebuffer.h"
 
 #include "src/gpu/vk/GrVkAttachment.h"
+#include "src/gpu/vk/GrVkCommandBuffer.h"
 #include "src/gpu/vk/GrVkGpu.h"
 #include "src/gpu/vk/GrVkImageView.h"
 #include "src/gpu/vk/GrVkRenderPass.h"
@@ -16,9 +17,9 @@
         GrVkGpu* gpu,
         int width, int height,
         const GrVkRenderPass* renderPass,
-        const GrVkAttachment* colorAttachment,
-        const GrVkAttachment* resolveAttachment,
-        const GrVkAttachment* stencilAttachment,
+        GrVkAttachment* colorAttachment,
+        GrVkAttachment* resolveAttachment,
+        GrVkAttachment* stencilAttachment,
         GrVkResourceProvider::CompatibleRPHandle compatibleRenderPassHandle) {
     // At the very least we need a renderPass and a colorAttachment
     SkASSERT(renderPass);
@@ -59,9 +60,57 @@
                                compatibleRenderPassHandle);
 }
 
+GrVkFramebuffer::GrVkFramebuffer(const GrVkGpu* gpu,
+                                 VkFramebuffer framebuffer,
+                                 sk_sp<GrVkAttachment> colorAttachment,
+                                 sk_sp<GrVkAttachment> resolveAttachment,
+                                 sk_sp<GrVkAttachment> stencilAttachment,
+                                 GrVkResourceProvider::CompatibleRPHandle compatibleRPHandle)
+        : GrVkManagedResource(gpu)
+        , fFramebuffer(framebuffer)
+        , fColorAttachment(std::move(colorAttachment))
+        , fResolveAttachment(std::move(resolveAttachment))
+        , fStencilAttachment(std::move(stencilAttachment))
+        , fCompatibleRenderPassHandle(compatibleRPHandle) {}
+
+GrVkFramebuffer::GrVkFramebuffer(const GrVkGpu* gpu,
+                                 sk_sp<GrVkAttachment> colorAttachment,
+                                 sk_sp<const GrVkRenderPass> renderPass,
+                                 std::unique_ptr<GrVkSecondaryCommandBuffer> externalCommandBuffer)
+        : GrVkManagedResource(gpu)
+        , fColorAttachment(std::move(colorAttachment))
+        , fExternalRenderPass(std::move(renderPass))
+        , fExternalCommandBuffer(std::move(externalCommandBuffer)) {}
+
 GrVkFramebuffer::~GrVkFramebuffer() {}
 
 void GrVkFramebuffer::freeGPUData() const {
-    SkASSERT(fFramebuffer);
-    GR_VK_CALL(fGpu->vkInterface(), DestroyFramebuffer(fGpu->device(), fFramebuffer, nullptr));
+    SkASSERT(this->isExternal() || fFramebuffer != VK_NULL_HANDLE);
+    if (!this->isExternal()) {
+        GR_VK_CALL(fGpu->vkInterface(), DestroyFramebuffer(fGpu->device(), fFramebuffer, nullptr));
+    }
+
+    // TODO: having freeGPUData virtual on GrManagedResource be const seems like a bad restriction
+    // since we are changing the internal objects of these classes when it is called. We should go
+    // back a revisit how much of a headache it would be to make this function non-const
+    GrVkFramebuffer* nonConstThis = const_cast<GrVkFramebuffer*>(this);
+    nonConstThis->releaseResources();
+}
+
+void GrVkFramebuffer::releaseResources() {
+    if (fExternalCommandBuffer) {
+        fExternalCommandBuffer->releaseResources();
+        fExternalCommandBuffer.reset();
+    }
+}
+
+void GrVkFramebuffer::returnExternalGrSecondaryCommandBuffer(
+        std::unique_ptr<GrVkSecondaryCommandBuffer> cmdBuffer) {
+    SkASSERT(!fExternalCommandBuffer);
+    fExternalCommandBuffer = std::move(cmdBuffer);
+}
+
+std::unique_ptr<GrVkSecondaryCommandBuffer> GrVkFramebuffer::externalCommandBuffer() {
+    SkASSERT(fExternalCommandBuffer);
+    return std::move(fExternalCommandBuffer);
 }
diff --git a/src/gpu/vk/GrVkFramebuffer.h b/src/gpu/vk/GrVkFramebuffer.h
index 2f1aa19..e694917 100644
--- a/src/gpu/vk/GrVkFramebuffer.h
+++ b/src/gpu/vk/GrVkFramebuffer.h
@@ -23,13 +23,34 @@
     static GrVkFramebuffer* Create(GrVkGpu* gpu,
                                    int width, int height,
                                    const GrVkRenderPass* renderPass,
-                                   const GrVkAttachment* colorAttachment,
-                                   const GrVkAttachment* resolveAttachment,
-                                   const GrVkAttachment* stencilAttachment,
+                                   GrVkAttachment* colorAttachment,
+                                   GrVkAttachment* resolveAttachment,
+                                   GrVkAttachment* stencilAttachment,
                                    GrVkResourceProvider::CompatibleRPHandle);
 
+    // Used for wrapped external secondary command buffers
+    GrVkFramebuffer(const GrVkGpu* gpu,
+                    sk_sp<GrVkAttachment> colorAttachment,
+                    sk_sp<const GrVkRenderPass> renderPass,
+                    std::unique_ptr<GrVkSecondaryCommandBuffer>);
+
     VkFramebuffer framebuffer() const { return fFramebuffer; }
 
+    const GrVkRenderPass* externalRenderPass() const { return fExternalRenderPass.get(); }
+    std::unique_ptr<GrVkSecondaryCommandBuffer> externalCommandBuffer();
+
+    // When we wrap a secondary command buffer, we will record GrManagedResources onto it which need
+    // to be kept alive till the command buffer gets submitted and the GPU has finished. However, in
+    // the wrapped case, we don't know when the command buffer gets submitted and when it is
+    // finished on the GPU since the client is in charge of that. However, we do require that the
+    // client keeps the GrVkSecondaryCBDrawContext alive and call releaseResources on it once the
+    // GPU is finished all the work. Thus we can use this to manage the lifetime of our
+    // GrVkSecondaryCommandBuffers. By storing them on the external GrVkFramebuffer owned by the
+    // GrVkRenderTarget, which is owned by the SkGpuDevice on the GrVkSecondaryCBDrawContext, we
+    // assure that the GrManagedResources held by the GrVkSecondaryCommandBuffer don't get deleted
+    // before they are allowed to.
+    void returnExternalGrSecondaryCommandBuffer(std::unique_ptr<GrVkSecondaryCommandBuffer>);
+
 #ifdef SK_TRACE_MANAGED_RESOURCES
     void dumpInfo() const override {
         SkDebugf("GrVkFramebuffer: %d (%d refs)\n", fFramebuffer, this->getRefCnt());
@@ -40,33 +61,35 @@
         return fCompatibleRenderPassHandle;
     }
 
+    GrVkAttachment* colorAttachment() { return fColorAttachment.get(); }
+    GrVkAttachment* resolveAttachment() { return fResolveAttachment.get(); }
+    GrVkAttachment* stencilAttachment() { return fStencilAttachment.get(); }
+
 private:
     GrVkFramebuffer(const GrVkGpu* gpu,
                     VkFramebuffer framebuffer,
                     sk_sp<GrVkAttachment> colorAttachment,
                     sk_sp<GrVkAttachment> resolveAttachment,
                     sk_sp<GrVkAttachment> stencilAttachment,
-                    GrVkResourceProvider::CompatibleRPHandle compatibleRenderPassHandle)
-        : INHERITED(gpu)
-        , fFramebuffer(framebuffer)
-        , fColorAttachment(std::move(colorAttachment))
-        , fResolveAttachment(std::move(resolveAttachment))
-        , fStencilAttachment(std::move(stencilAttachment))
-        , fCompatibleRenderPassHandle(compatibleRenderPassHandle) {}
+                    GrVkResourceProvider::CompatibleRPHandle);
 
     ~GrVkFramebuffer() override;
 
+    bool isExternal() const { return fExternalCommandBuffer.get(); }
+
     void freeGPUData() const override;
+    void releaseResources();
 
-    VkFramebuffer  fFramebuffer;
+    VkFramebuffer  fFramebuffer = VK_NULL_HANDLE;
 
-    sk_sp<const GrVkAttachment> fColorAttachment;
-    sk_sp<const GrVkAttachment> fResolveAttachment;
-    sk_sp<const GrVkAttachment> fStencilAttachment;
+    sk_sp<GrVkAttachment> fColorAttachment;
+    sk_sp<GrVkAttachment> fResolveAttachment;
+    sk_sp<GrVkAttachment> fStencilAttachment;
 
     GrVkResourceProvider::CompatibleRPHandle fCompatibleRenderPassHandle;
 
-    using INHERITED = GrVkManagedResource;
+    sk_sp<const GrVkRenderPass> fExternalRenderPass;
+    std::unique_ptr<GrVkSecondaryCommandBuffer> fExternalCommandBuffer;
 };
 
 #endif
diff --git a/src/gpu/vk/GrVkOpsRenderPass.cpp b/src/gpu/vk/GrVkOpsRenderPass.cpp
index b082527..7de5f7d 100644
--- a/src/gpu/vk/GrVkOpsRenderPass.cpp
+++ b/src/gpu/vk/GrVkOpsRenderPass.cpp
@@ -291,12 +291,10 @@
     SkASSERT(fCurrentRenderPass);
     fCurrentRenderPass->ref();
 
-    fCurrentSecondaryCommandBuffer.reset(
-            GrVkSecondaryCommandBuffer::Create(vkRT->getExternalSecondaryCommandBuffer()));
+    fCurrentSecondaryCommandBuffer = vkRT->externalCommandBuffer();
     if (!fCurrentSecondaryCommandBuffer) {
         return false;
     }
-    fCurrentSecondaryCommandBuffer->begin(fGpu, nullptr, fCurrentRenderPass);
     return true;
 }
 
@@ -344,7 +342,7 @@
         // We pass the ownership of the GrVkSecondaryCommandBuffer to the special wrapped
         // GrVkRenderTarget since it's lifetime matches the lifetime we need to keep the
         // GrManagedResources on the GrVkSecondaryCommandBuffer alive.
-        static_cast<GrVkRenderTarget*>(fRenderTarget)->addWrappedGrSecondaryCommandBuffer(
+        static_cast<GrVkRenderTarget*>(fRenderTarget)->returnExternalGrSecondaryCommandBuffer(
                 std::move(fCurrentSecondaryCommandBuffer));
         return;
     }
@@ -652,7 +650,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 void GrVkOpsRenderPass::onEnd() {
-    if (fCurrentSecondaryCommandBuffer) {
+    if (fCurrentSecondaryCommandBuffer && !this->wrapsSecondaryCommandBuffer()) {
         fCurrentSecondaryCommandBuffer->end(fGpu);
     }
 }
diff --git a/src/gpu/vk/GrVkRenderTarget.cpp b/src/gpu/vk/GrVkRenderTarget.cpp
index 6dc077a..f6aeaf0 100644
--- a/src/gpu/vk/GrVkRenderTarget.cpp
+++ b/src/gpu/vk/GrVkRenderTarget.cpp
@@ -75,31 +75,31 @@
 
 GrVkRenderTarget::GrVkRenderTarget(GrVkGpu* gpu,
                                    SkISize dimensions,
-                                   sk_sp<GrVkAttachment> colorAttachment,
-                                   const GrVkRenderPass* renderPass,
-                                   VkCommandBuffer secondaryCommandBuffer)
+                                   sk_sp<GrVkFramebuffer> externalFramebuffer)
         : GrSurface(gpu, dimensions,
-                    colorAttachment->isProtected() ? GrProtected::kYes : GrProtected::kNo)
+                    externalFramebuffer->colorAttachment()->isProtected() ? GrProtected::kYes
+                                                                          : GrProtected::kNo)
         , GrRenderTarget(gpu, dimensions, 1,
-                         colorAttachment->isProtected() ? GrProtected::kYes : GrProtected::kNo)
-        , fColorAttachment(std::move(colorAttachment))
+                         externalFramebuffer->colorAttachment()->isProtected() ? GrProtected::kYes
+                                                                               : GrProtected::kNo)
         , fCachedFramebuffers()
         , fCachedRenderPasses()
-        , fSecondaryCommandBuffer(secondaryCommandBuffer) {
-    SkASSERT(fColorAttachment->numSamples() == 1);
-    SkASSERT(fSecondaryCommandBuffer != VK_NULL_HANDLE);
-    SkASSERT(SkToBool(fColorAttachment->vkUsageFlags() & VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT));
-    SkASSERT(!SkToBool(fColorAttachment->vkUsageFlags() & VK_IMAGE_USAGE_INPUT_ATTACHMENT_BIT));
+        , fExternalFramebuffer(externalFramebuffer) {
+    SkASSERT(fExternalFramebuffer);
+    SkASSERT(!fColorAttachment);
+    SkDEBUGCODE(auto colorAttachment = fExternalFramebuffer->colorAttachment());
+    SkASSERT(colorAttachment);
+    SkASSERT(colorAttachment->numSamples() == 1);
+    SkASSERT(SkToBool(colorAttachment->vkUsageFlags() & VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT));
+    SkASSERT(!SkToBool(colorAttachment->vkUsageFlags() & VK_IMAGE_USAGE_INPUT_ATTACHMENT_BIT));
     this->setFlags();
     this->registerWithCacheWrapped(GrWrapCacheable::kNo);
-    // We use the cached renderpass with no stencil and no extra dependencies to hold the external
-    // render pass.
-    int exteralRPIndex = renderpass_features_to_index(false, false, SelfDependencyFlags::kNone,
-                                                      LoadFromResolve::kNo);
-    fCachedRenderPasses[exteralRPIndex] = renderPass;
 }
 
 void GrVkRenderTarget::setFlags() {
+    if (this->wrapsSecondaryCommandBuffer()) {
+        return;
+    }
     GrVkAttachment* nonMSAAAttachment = this->nonMSAAAttachment();
     if (nonMSAAAttachment && nonMSAAAttachment->supportsInputAttachmentUsage()) {
         this->setVkRTSupportsInputAttachment();
@@ -164,17 +164,33 @@
     sk_sp<GrBackendSurfaceMutableStateImpl> mutableState(new GrBackendSurfaceMutableStateImpl(
             VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, VK_QUEUE_FAMILY_IGNORED));
 
-    sk_sp<GrVkAttachment> scbAttachment =
+    sk_sp<GrVkAttachment> colorAttachment =
         GrVkAttachment::MakeWrapped(gpu, dimensions, info, std::move(mutableState),
                                     GrAttachment::UsageFlags::kColorAttachment,
                                     kBorrow_GrWrapOwnership, GrWrapCacheable::kNo, true);
 
-    GrVkRenderTarget* vkRT = new GrVkRenderTarget(gpu, dimensions, std::move(scbAttachment),
-                                                  rp, vkInfo.fSecondaryCommandBuffer);
+    std::unique_ptr<GrVkSecondaryCommandBuffer> scb(
+            GrVkSecondaryCommandBuffer::Create(vkInfo.fSecondaryCommandBuffer, rp));
+    if (!scb) {
+        return nullptr;
+    }
+
+    sk_sp<GrVkFramebuffer> framebuffer(new GrVkFramebuffer(
+            gpu, std::move(colorAttachment), sk_sp<const GrVkRenderPass>(rp),
+            std::move(scb)));
+
+    GrVkRenderTarget* vkRT = new GrVkRenderTarget(gpu, dimensions, std::move(framebuffer));
 
     return sk_sp<GrVkRenderTarget>(vkRT);
 }
 
+GrBackendFormat GrVkRenderTarget::backendFormat() const {
+    if (this->wrapsSecondaryCommandBuffer()) {
+        return fExternalFramebuffer->colorAttachment()->getBackendFormat();
+    }
+    return fColorAttachment->getBackendFormat();
+}
+
 GrVkAttachment* GrVkRenderTarget::nonMSAAAttachment() const {
     if (fColorAttachment->numSamples() == 1) {
         return fColorAttachment.get();
@@ -188,15 +204,23 @@
     return true;
 }
 
+std::unique_ptr<GrVkSecondaryCommandBuffer>
+GrVkRenderTarget::externalCommandBuffer() const {
+    SkASSERT(this->wrapsSecondaryCommandBuffer());
+    return fExternalFramebuffer->externalCommandBuffer();
+}
+
 const GrVkRenderPass* GrVkRenderTarget::externalRenderPass() const {
     SkASSERT(this->wrapsSecondaryCommandBuffer());
-    // We use the cached render pass with no attachments or self dependencies to hold the
-    // external render pass.
-    int exteralRPIndex = renderpass_features_to_index(false, false, SelfDependencyFlags::kNone,
-                                                      LoadFromResolve::kNo);
-    return fCachedRenderPasses[exteralRPIndex];
+    return fExternalFramebuffer->externalRenderPass();
 }
 
+void GrVkRenderTarget::returnExternalGrSecondaryCommandBuffer(
+        std::unique_ptr<GrVkSecondaryCommandBuffer> cmdBuffer) {
+    fExternalFramebuffer->returnExternalGrSecondaryCommandBuffer(std::move(cmdBuffer));
+}
+
+
 GrVkResourceProvider::CompatibleRPHandle GrVkRenderTarget::compatibleRenderPassHandle(
         bool withResolve,
         bool withStencil,
@@ -231,17 +255,17 @@
                                               bool withStencil,
                                               SelfDependencyFlags selfDepFlags,
                                               LoadFromResolve loadFromResolve) {
-    int cacheIndex = renderpass_features_to_index(withResolve, withStencil, selfDepFlags,
-                                                  loadFromResolve);
-    SkASSERT(cacheIndex < GrVkRenderTarget::kNumCachedRenderPasses);
-    const GrVkRenderPass* rp = fCachedRenderPasses[cacheIndex];
     if (this->wrapsSecondaryCommandBuffer()) {
-        SkASSERT(rp);
         // The compatible handle is invalid for external render passes used in wrapped secondary
         // command buffers. However, this should not be called by code using external render passes
         // that needs to use the handle.
-        return {rp, GrVkResourceProvider::CompatibleRPHandle()};
+        return {fExternalFramebuffer->externalRenderPass(),
+                GrVkResourceProvider::CompatibleRPHandle()};
     }
+    int cacheIndex =
+            renderpass_features_to_index(withResolve, withStencil, selfDepFlags, loadFromResolve);
+    SkASSERT(cacheIndex < GrVkRenderTarget::kNumCachedRenderPasses);
+    const GrVkRenderPass* rp = fCachedRenderPasses[cacheIndex];
     if (!rp) {
         rp = this->createSimpleRenderPass(withResolve, withStencil, selfDepFlags, loadFromResolve);
     }
@@ -298,11 +322,11 @@
             renderpass_features_to_index(withResolve, withStencil, selfDepFlags, loadFromResolve);
     SkASSERT(cacheIndex < GrVkRenderTarget::kNumCachedRenderPasses);
 
-    const GrVkAttachment* resolve = withResolve ? this->resolveAttachment() : nullptr;
+    GrVkAttachment* resolve = withResolve ? this->resolveAttachment() : nullptr;
 
     // Stencil attachment view is stored in the base RT stencil attachment
-    const GrVkAttachment* stencil =
-            withStencil ? static_cast<const GrVkAttachment*>(this->getStencilAttachment())
+    GrVkAttachment* stencil =
+            withStencil ? static_cast<GrVkAttachment*>(this->getStencilAttachment())
                         : nullptr;
     fCachedFramebuffers[cacheIndex] =
             GrVkFramebuffer::Create(gpu, this->width(), this->height(), renderPass,
@@ -457,11 +481,7 @@
         fCachedInputDescriptorSet = nullptr;
     }
 
-    for (int i = 0; i < fGrSecondaryCommandBuffers.count(); ++i) {
-        SkASSERT(fGrSecondaryCommandBuffers[i]);
-        fGrSecondaryCommandBuffers[i]->releaseResources();
-    }
-    fGrSecondaryCommandBuffers.reset();
+    fExternalFramebuffer.reset();
 }
 
 void GrVkRenderTarget::onRelease() {
diff --git a/src/gpu/vk/GrVkRenderTarget.h b/src/gpu/vk/GrVkRenderTarget.h
index bb2c047..73e2aab 100644
--- a/src/gpu/vk/GrVkRenderTarget.h
+++ b/src/gpu/vk/GrVkRenderTarget.h
@@ -13,7 +13,6 @@
 #include "src/gpu/vk/GrVkImage.h"
 
 #include "include/gpu/vk/GrVkTypes.h"
-#include "src/gpu/vk/GrVkCommandBuffer.h"
 #include "src/gpu/vk/GrVkRenderPass.h"
 #include "src/gpu/vk/GrVkResourceProvider.h"
 
@@ -24,18 +23,21 @@
 
 struct GrVkImageInfo;
 
-class GrVkRenderTarget: public GrRenderTarget {
+class GrVkRenderTarget : public GrRenderTarget {
 public:
-    static sk_sp<GrVkRenderTarget> MakeWrappedRenderTarget(GrVkGpu*, SkISize, int sampleCnt,
+    static sk_sp<GrVkRenderTarget> MakeWrappedRenderTarget(GrVkGpu*,
+                                                           SkISize,
+                                                           int sampleCnt,
                                                            const GrVkImageInfo&,
                                                            sk_sp<GrBackendSurfaceMutableStateImpl>);
 
-    static sk_sp<GrVkRenderTarget> MakeSecondaryCBRenderTarget(GrVkGpu*, SkISize,
+    static sk_sp<GrVkRenderTarget> MakeSecondaryCBRenderTarget(GrVkGpu*,
+                                                               SkISize,
                                                                const GrVkDrawableInfo& vkInfo);
 
     ~GrVkRenderTarget() override;
 
-    GrBackendFormat backendFormat() const override { return fColorAttachment->getBackendFormat(); }
+    GrBackendFormat backendFormat() const override;
 
     using SelfDependencyFlags = GrVkRenderPass::SelfDependencyFlags;
     using LoadFromResolve = GrVkRenderPass::LoadFromResolve;
@@ -51,11 +53,21 @@
                                     renderPass.loadFromResolve());
     }
 
-    GrVkAttachment* colorAttachment() const { return fColorAttachment.get(); }
-    const GrVkImageView* colorAttachmentView() const { return fColorAttachment->framebufferView(); }
+    GrVkAttachment* colorAttachment() const {
+        SkASSERT(!this->wrapsSecondaryCommandBuffer());
+        return fColorAttachment.get();
+    }
+    const GrVkImageView* colorAttachmentView() const {
+        SkASSERT(!this->wrapsSecondaryCommandBuffer());
+        return fColorAttachment->framebufferView();
+    }
 
-    GrVkAttachment* resolveAttachment() const { return fResolveAttachment.get(); }
+    GrVkAttachment* resolveAttachment() const {
+        SkASSERT(!this->wrapsSecondaryCommandBuffer());
+        return fResolveAttachment.get();
+    }
     const GrVkImageView* resolveAttachmentView() const {
+        SkASSERT(!this->wrapsSecondaryCommandBuffer());
         return fResolveAttachment->framebufferView();
     }
 
@@ -84,12 +96,11 @@
             bool withStencil,
             SelfDependencyFlags selfDepFlags,
             LoadFromResolve);
-    const GrVkRenderPass* externalRenderPass() const;
 
-    bool wrapsSecondaryCommandBuffer() const { return fSecondaryCommandBuffer != VK_NULL_HANDLE; }
-    VkCommandBuffer getExternalSecondaryCommandBuffer() const {
-        return fSecondaryCommandBuffer;
-    }
+    bool wrapsSecondaryCommandBuffer() const { return SkToBool(fExternalFramebuffer); }
+    std::unique_ptr<GrVkSecondaryCommandBuffer> externalCommandBuffer() const;
+    void returnExternalGrSecondaryCommandBuffer(std::unique_ptr<GrVkSecondaryCommandBuffer>);
+    const GrVkRenderPass* externalRenderPass() const;
 
     bool canAttemptStencilAttachment() const override {
         // We don't know the status of the stencil attachment for wrapped external secondary command
@@ -118,10 +129,6 @@
     // alive.
     const GrVkDescriptorSet* inputDescSet(GrVkGpu*, bool forResolve);
 
-    void addWrappedGrSecondaryCommandBuffer(std::unique_ptr<GrVkSecondaryCommandBuffer> cmdBuffer) {
-        fGrSecondaryCommandBuffers.push_back(std::move(cmdBuffer));
-    }
-
 protected:
     enum class CreateType {
         kDirectlyWrapped, // We need to register this in the ctor
@@ -141,11 +148,10 @@
     size_t onGpuMemorySize() const override { return 0; }
 
 private:
+    // For external framebuffers that wrap a secondary command buffer
     GrVkRenderTarget(GrVkGpu* gpu,
                      SkISize dimensions,
-                     sk_sp<GrVkAttachment> colorAttachment,
-                     const GrVkRenderPass* renderPass,
-                     VkCommandBuffer secondaryCommandBuffer);
+                     sk_sp<GrVkFramebuffer> externalFramebuffer);
 
     void setFlags();
 
@@ -188,20 +194,7 @@
 
     const GrVkDescriptorSet* fCachedInputDescriptorSet = nullptr;
 
-    // If this render target wraps an external VkCommandBuffer, then this handle will be that
-    // VkCommandBuffer and not VK_NULL_HANDLE. In this case the render target will not be backed by
-    // an actual VkImage and will thus be limited in terms of what it can be used for.
-    VkCommandBuffer fSecondaryCommandBuffer = VK_NULL_HANDLE;
-    // When we wrap a secondary command buffer, we will record GrManagedResources onto it which need
-    // to be kept alive till the command buffer gets submitted and the GPU has finished. However, in
-    // the wrapped case, we don't know when the command buffer gets submitted and when it is
-    // finished on the GPU since the client is in charge of that. However, we do require that the
-    // client keeps the GrVkSecondaryCBDrawContext alive and call releaseResources on it once the
-    // GPU is finished all the work. Thus we can use this to manage the lifetime of our
-    // GrVkSecondaryCommandBuffers. By storing them on the GrVkRenderTarget, which is owned by the
-    // SkGpuDevice on the GrVkSecondaryCBDrawContext, we assure that the GrManagedResources held by
-    // the GrVkSecondaryCommandBuffer don't get deleted before they are allowed to.
-    SkTArray<std::unique_ptr<GrVkCommandBuffer>> fGrSecondaryCommandBuffers;
+    sk_sp<GrVkFramebuffer> fExternalFramebuffer;
 };
 
 #endif