Store discard request on the opList and remove GrDiscardOp (take 2)

Change-Id: I2f1bd6f8547895cc8d66cfde42d7d890441d198e
Reviewed-on: https://skia-review.googlesource.com/33460
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>

9 files changed, 79 insertions, 53 deletions

diff --git a/gn/gpu.gni b/gn/gpu.gni
index 51ac125366..57564e9fb3 100644
--- a/gn/gpu.gni
+++ b/gn/gpu.gni
@@ -247,7 +247,6 @@ skia_gpu_sources = [
   "$_src/gpu/ops/GrDashOp.h",
   "$_src/gpu/ops/GrDefaultPathRenderer.cpp",
   "$_src/gpu/ops/GrDefaultPathRenderer.h",
-  "$_src/gpu/ops/GrDiscardOp.h",
   "$_src/gpu/ops/GrDebugMarkerOp.h",
   "$_src/gpu/ops/GrDrawAtlasOp.cpp",
   "$_src/gpu/ops/GrDrawAtlasOp.h",
diff --git a/include/private/GrTypesPriv.h b/include/private/GrTypesPriv.h
index b9b2cf7807..41a2deaecf 100644
--- a/include/private/GrTypesPriv.h
+++ b/include/private/GrTypesPriv.h
@@ -23,6 +23,23 @@ using GrStdSteadyClock = std::chrono::monotonic_clock;
 using GrStdSteadyClock = std::chrono::steady_clock;
 #endif
 
+/** This enum is used to specify the load operation to be used when an
+ *  opList/GrGpuCommandBuffer begins execution.
+ */
+enum class GrLoadOp {
+    kLoad,
+    kClear,
+    kDiscard,
+};
+
+/** This enum is used to specify the store operation to be used when an
+ *  opList/GrGpuCommandBuffer ends execution.
+ */
+enum class GrStoreOp {
+    kStore,
+    kDiscard,
+};
+
 /** This enum indicates the type of antialiasing to be performed. */
 enum class GrAAType : unsigned {
     /** No antialiasing */
diff --git a/src/gpu/GrGpuCommandBuffer.h b/src/gpu/GrGpuCommandBuffer.h
index 07e823fd11..a80f6e494e 100644
--- a/src/gpu/GrGpuCommandBuffer.h
+++ b/src/gpu/GrGpuCommandBuffer.h
@@ -71,28 +71,17 @@ private:
  */
 class GrGpuRTCommandBuffer : public GrGpuCommandBuffer {
 public:
-    enum class LoadOp {
-        kLoad,
-        kClear,
-        kDiscard,
-    };
-
-    enum class StoreOp {
-        kStore,
-        kDiscard,
-    };
-
     struct LoadAndStoreInfo {
-        LoadOp  fLoadOp;
-        StoreOp fStoreOp;
-        GrColor fClearColor;
+        GrLoadOp  fLoadOp;
+        GrStoreOp fStoreOp;
+        GrColor   fClearColor;
     };
 
     // Load-time clears of the stencil buffer are always to 0 so we don't store
     // an 'fStencilClearValue'
     struct StencilLoadAndStoreInfo {
-        LoadOp fLoadOp;
-        StoreOp fStoreOp;
+        GrLoadOp  fLoadOp;
+        GrStoreOp fStoreOp;
     };
 
     virtual ~GrGpuRTCommandBuffer() {}
diff --git a/src/gpu/GrOpList.h b/src/gpu/GrOpList.h
index a9fa7bdc49..50eaaef31f 100644
--- a/src/gpu/GrOpList.h
+++ b/src/gpu/GrOpList.h
@@ -8,6 +8,7 @@
 #ifndef GrOpList_DEFINED
 #define GrOpList_DEFINED
 
+#include "GrColor.h"
 #include "GrGpuResourceRef.h"
 #include "SkRefCnt.h"
 #include "SkTDArray.h"
@@ -92,9 +93,6 @@ public:
 
     int32_t uniqueID() const { return fUniqueID; }
 
-    void setRequiresStencil() { this->setFlag(kClearStencilBuffer_Flag); }
-    bool requiresStencil() { return this->isSetFlag(kClearStencilBuffer_Flag); }
-
     /*
      * Dump out the GrOpList dependency DAG
      */
@@ -103,10 +101,18 @@ public:
     SkDEBUGCODE(virtual int numOps() const = 0;)
     SkDEBUGCODE(virtual int numClips() const { return 0; })
 
+    void setColorLoadOp(GrLoadOp loadOp) { fColorLoadOp = loadOp; }
+    void setLoadClearColor(GrColor color) { fLoadClearColor = color; }
+    void setStencilLoadOp(GrLoadOp loadOp) { fStencilLoadOp = loadOp; }
+
 protected:
     GrSurfaceProxyRef fTarget;
     GrAuditTrail*     fAuditTrail;
 
+    GrLoadOp          fColorLoadOp    = GrLoadOp::kLoad;
+    GrColor           fLoadClearColor = 0x0;
+    GrLoadOp          fStencilLoadOp  = GrLoadOp::kLoad;
+
 private:
     friend class GrDrawingManager; // for resetFlag & TopoSortTraits
 
@@ -117,8 +123,6 @@ private:
 
         kWasOutput_Flag = 0x02,   //!< Flag for topological sorting
         kTempMark_Flag  = 0x04,   //!< Flag for topological sorting
-
-        kClearStencilBuffer_Flag = 0x08 //!< Clear the SB before executing the ops
     };
 
     void setFlag(uint32_t flag) {
diff --git a/src/gpu/GrRenderTargetContext.cpp b/src/gpu/GrRenderTargetContext.cpp
index 39515adc0a..d3c9d9b3f0 100644
--- a/src/gpu/GrRenderTargetContext.cpp
+++ b/src/gpu/GrRenderTargetContext.cpp
@@ -31,7 +31,6 @@
 #include "ops/GrClearOp.h"
 #include "ops/GrClearStencilClipOp.h"
 #include "ops/GrDebugMarkerOp.h"
-#include "ops/GrDiscardOp.h"
 #include "ops/GrDrawAtlasOp.h"
 #include "ops/GrDrawOp.h"
 #include "ops/GrDrawVerticesOp.h"
@@ -216,18 +215,25 @@ void GrRenderTargetContext::discard() {
     ASSERT_SINGLE_OWNER
     RETURN_IF_ABANDONED
     SkDEBUGCODE(this->validate();)
-            GR_CREATE_TRACE_MARKER_CONTEXT("GrRenderTargetContext", "discard", fContext);
+    GR_CREATE_TRACE_MARKER_CONTEXT("GrRenderTargetContext", "discard", fContext);
 
     AutoCheckFlush acf(this->drawingManager());
 
-    // Currently this just inserts a discard op. However, once in MDB this can remove all the
-    // previously recorded ops and change the load op to discard.
-    if (this->caps()->discardRenderTargetSupport()) {
-        std::unique_ptr<GrOp> op(GrDiscardOp::Make(fRenderTargetProxy.get()));
-        if (!op) {
-            return;
+    // Discard calls to in-progress opLists are ignored. Calls at the start update the
+    // opLists' color & stencil load ops.
+    if (this->getRTOpList()->isEmpty()) {
+        if (this->caps()->discardRenderTargetSupport()) {
+            this->getRTOpList()->setColorLoadOp(GrLoadOp::kDiscard);
+            this->getRTOpList()->setStencilLoadOp(GrLoadOp::kDiscard);
+        } else {
+            // skbug.com/6956 (Extra clear confuses Nexus7)
+#if 0
+            // Surely, if a discard was requested, a clear should be acceptable
+            this->getRTOpList()->setColorLoadOp(GrLoadOp::kClear);
+            this->getRTOpList()->setLoadClearColor(0x0);
+            this->getRTOpList()->setStencilLoadOp(GrLoadOp::kClear);
+#endif
         }
-        this->getRTOpList()->addOp(std::move(op), *this->caps());
     }
 }
 
@@ -1751,7 +1757,7 @@ uint32_t GrRenderTargetContext::addDrawOp(const GrClip& clip, std::unique_ptr<Gr
 
     if (fixedFunctionFlags & GrDrawOp::FixedFunctionFlags::kUsesStencil ||
         appliedClip.hasStencilClip()) {
-        this->getOpList()->setRequiresStencil();
+        this->getOpList()->setStencilLoadOp(GrLoadOp::kClear);
 
         this->setNeedsStencil();
     }
diff --git a/src/gpu/GrRenderTargetOpList.cpp b/src/gpu/GrRenderTargetOpList.cpp
index b947891d90..30bd30d16b 100644
--- a/src/gpu/GrRenderTargetOpList.cpp
+++ b/src/gpu/GrRenderTargetOpList.cpp
@@ -94,11 +94,13 @@ void GrRenderTargetOpList::onPrepare(GrOpFlushState* flushState) {
 static std::unique_ptr<GrGpuRTCommandBuffer> create_command_buffer(GrGpu* gpu,
                                                                    GrRenderTarget* rt,
                                                                    GrSurfaceOrigin origin,
-                                                                   bool clearSB) {
+                                                                   GrLoadOp colorLoadOp,
+                                                                   GrColor loadClearColor,
+                                                                   GrLoadOp stencilLoadOp) {
     static const GrGpuRTCommandBuffer::LoadAndStoreInfo kBasicLoadStoreInfo {
-        GrGpuRTCommandBuffer::LoadOp::kLoad,
-        GrGpuRTCommandBuffer::StoreOp::kStore,
-        GrColor_ILLEGAL
+        colorLoadOp,
+        GrStoreOp::kStore,
+        loadClearColor
     };
 
     // TODO:
@@ -107,8 +109,8 @@ static std::unique_ptr<GrGpuRTCommandBuffer> create_command_buffer(GrGpu* gpu,
     // Note: we would still need SB loads and stores but they would happen at a
     // lower level (inside the VK command buffer).
     const GrGpuRTCommandBuffer::StencilLoadAndStoreInfo stencilLoadAndStoreInfo {
-        clearSB ? GrGpuRTCommandBuffer::LoadOp::kClear : GrGpuRTCommandBuffer::LoadOp::kLoad,
-        GrGpuRTCommandBuffer::StoreOp::kStore,
+        stencilLoadOp,
+        GrStoreOp::kStore,
     };
 
     std::unique_ptr<GrGpuRTCommandBuffer> buffer(
@@ -140,11 +142,14 @@ bool GrRenderTargetOpList::onExecute(GrOpFlushState* flushState) {
     TRACE_EVENT0("skia", TRACE_FUNC);
 #endif
 
+    // TODO: at the very least, we want the stencil store op to always be discard (at this
+    // level). In Vulkan, sub-command buffers would still need to load & store the stencil buffer.
     std::unique_ptr<GrGpuRTCommandBuffer> commandBuffer = create_command_buffer(
                                                     flushState->gpu(),
                                                     fTarget.get()->priv().peekRenderTarget(),
                                                     fTarget.get()->origin(),
-                                                    this->requiresStencil());
+                                                    fColorLoadOp, fLoadClearColor,
+                                                    fStencilLoadOp);
     flushState->setCommandBuffer(commandBuffer.get());
     commandBuffer->begin();
 
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index 594312071f..8e0490d729 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -2429,9 +2429,9 @@ bool GrGLGpu::onReadPixels(GrSurface* surface,
 
 GrGpuRTCommandBuffer* GrGLGpu::createCommandBuffer(
         GrRenderTarget* rt, GrSurfaceOrigin origin,
-        const GrGpuRTCommandBuffer::LoadAndStoreInfo&,
+        const GrGpuRTCommandBuffer::LoadAndStoreInfo& colorInfo,
         const GrGpuRTCommandBuffer::StencilLoadAndStoreInfo& stencilInfo) {
-    return new GrGLGpuRTCommandBuffer(this, rt, origin, stencilInfo);
+    return new GrGLGpuRTCommandBuffer(this, rt, origin, colorInfo, stencilInfo);
 }
 
 GrGpuTextureCommandBuffer* GrGLGpu::createCommandBuffer(GrTexture* texture,
diff --git a/src/gpu/gl/GrGLGpuCommandBuffer.h b/src/gpu/gl/GrGLGpuCommandBuffer.h
index e76253669c..f57935a1a5 100644
--- a/src/gpu/gl/GrGLGpuCommandBuffer.h
+++ b/src/gpu/gl/GrGLGpuCommandBuffer.h
@@ -50,16 +50,22 @@ class GrGLGpuRTCommandBuffer : public GrGpuRTCommandBuffer {
  */
 public:
     GrGLGpuRTCommandBuffer(GrGLGpu* gpu, GrRenderTarget* rt, GrSurfaceOrigin origin,
+                           const GrGpuRTCommandBuffer::LoadAndStoreInfo& colorInfo,
                            const GrGpuRTCommandBuffer::StencilLoadAndStoreInfo& stencilInfo)
             : INHERITED(rt, origin)
-            , fGpu(gpu) {
-        fClearSB = LoadOp::kClear == stencilInfo.fLoadOp;
+            , fGpu(gpu)
+            , fColorLoadAndStoreInfo(colorInfo)
+            , fStencilLoadAndStoreInfo(stencilInfo) {
     }
 
     ~GrGLGpuRTCommandBuffer() override {}
 
     void begin() override {
-        if (fClearSB) {
+        if (GrLoadOp::kClear == fColorLoadAndStoreInfo.fLoadOp) {
+            fGpu->clear(GrFixedClip::Disabled(), fColorLoadAndStoreInfo.fClearColor,
+                        fRenderTarget, fOrigin);
+        }
+        if (GrLoadOp::kClear == fStencilLoadAndStoreInfo.fLoadOp) {
             fGpu->clearStencil(fRenderTarget, 0x0);
         }
     }
@@ -102,8 +108,9 @@ private:
         fGpu->clearStencilClip(clip, insideStencilMask, fRenderTarget, fOrigin);
     }
 
-    GrGLGpu*                    fGpu;
-    bool                        fClearSB;
+    GrGLGpu*                                      fGpu;
+    GrGpuRTCommandBuffer::LoadAndStoreInfo        fColorLoadAndStoreInfo;
+    GrGpuRTCommandBuffer::StencilLoadAndStoreInfo fStencilLoadAndStoreInfo;
 
     typedef GrGpuRTCommandBuffer INHERITED;
 };
diff --git a/src/gpu/vk/GrVkGpuCommandBuffer.cpp b/src/gpu/vk/GrVkGpuCommandBuffer.cpp
index 4987cea3b1..1b2befff07 100644
--- a/src/gpu/vk/GrVkGpuCommandBuffer.cpp
+++ b/src/gpu/vk/GrVkGpuCommandBuffer.cpp
@@ -42,17 +42,16 @@ GrVkGpuTextureCommandBuffer::~GrVkGpuTextureCommandBuffer() {}
 
 ////////////////////////////////////////////////////////////////////////////////
 
-void get_vk_load_store_ops(GrGpuRTCommandBuffer::LoadOp loadOpIn,
-                           GrGpuRTCommandBuffer::StoreOp storeOpIn,
+void get_vk_load_store_ops(GrLoadOp loadOpIn, GrStoreOp storeOpIn,
                            VkAttachmentLoadOp* loadOp, VkAttachmentStoreOp* storeOp) {
     switch (loadOpIn) {
-        case GrGpuRTCommandBuffer::LoadOp::kLoad:
+        case GrLoadOp::kLoad:
             *loadOp = VK_ATTACHMENT_LOAD_OP_LOAD;
             break;
-        case GrGpuRTCommandBuffer::LoadOp::kClear:
+        case GrLoadOp::kClear:
             *loadOp = VK_ATTACHMENT_LOAD_OP_CLEAR;
             break;
-        case GrGpuRTCommandBuffer::LoadOp::kDiscard:
+        case GrLoadOp::kDiscard:
             *loadOp = VK_ATTACHMENT_LOAD_OP_DONT_CARE;
             break;
         default:
@@ -61,10 +60,10 @@ void get_vk_load_store_ops(GrGpuRTCommandBuffer::LoadOp loadOpIn,
     }
 
     switch (storeOpIn) {
-        case GrGpuRTCommandBuffer::StoreOp::kStore:
+        case GrStoreOp::kStore:
             *storeOp = VK_ATTACHMENT_STORE_OP_STORE;
             break;
-        case GrGpuRTCommandBuffer::StoreOp::kDiscard:
+        case GrStoreOp::kDiscard:
             *storeOp = VK_ATTACHMENT_STORE_OP_DONT_CARE;
             break;
         default: