From efd9e875db960302eeb19d91fe952762291f4255 Mon Sep 17 00:00:00 2001
From: Mathias Agopian <mathias@google.com>
Date: Mon, 6 Mar 2023 16:27:32 -0800
Subject: [PATCH] fix a discard flags issue

An attachment would be wrongly discarded if used as read-only: because
it didn't have a dependency to it, the discard flag was set.
This is fixed by setting the discard flag only if a resource is written
and has dependencies to it.

Should fix: #5005
---
 filament/src/fg/PassNode.cpp               |  9 +++-
 filament/test/filament_framegraph_test.cpp | 52 ++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/filament/src/fg/PassNode.cpp b/filament/src/fg/PassNode.cpp
index a28c2352a0..8515c53c97 100644
--- a/filament/src/fg/PassNode.cpp
+++ b/filament/src/fg/PassNode.cpp
@@ -151,7 +151,12 @@ void RenderPassNode::resolve() noexcept {
 
                 rt.targetBufferFlags |= target;
 
-                if (!rt.outgoing[i] || !rt.outgoing[i]->hasActiveReaders()) {
+                // Discard at the end only if we are writing to this attachment AND later reading
+                // from it. (in particular, don't discard if we're not writing at all, because this
+                // attachment might have other readers after us).
+                // TODO: we could set the discard flag if we are the last reader, i.e.
+                //       if rt->incoming[i] last reader is us.
+                if (rt.outgoing[i] && !rt.outgoing[i]->hasActiveReaders()) {
                     rt.backend.params.flags.discardEnd |= target;
                 }
                 if (!rt.outgoing[i] || !rt.outgoing[i]->hasWriterPass()) {
@@ -161,10 +166,10 @@ void RenderPassNode::resolve() noexcept {
                         rt.backend.params.readOnlyDepthStencil |= RenderPassParams::READONLY_STENCIL;
                     }
                 }
+                // Discard at the start if this attachment has no prior writer
                 if (!rt.incoming[i] || !rt.incoming[i]->hasActiveWriters()) {
                     rt.backend.params.flags.discardStart |= target;
                 }
-
                 VirtualResource* pResource = mFrameGraph.getResource(rt.descriptor.attachments.array[i]);
                 Resource<FrameGraphTexture>* pTextureResource = static_cast<Resource<FrameGraphTexture>*>(pResource);
 
diff --git a/filament/test/filament_framegraph_test.cpp b/filament/test/filament_framegraph_test.cpp
index d5463a9041..32eb3a6ed7 100644
--- a/filament/test/filament_framegraph_test.cpp
+++ b/filament/test/filament_framegraph_test.cpp
@@ -890,3 +890,55 @@ TEST_F(FrameGraphTest, SubResourcesWriteRead) {
 
     fg.execute(driverApi);
 }
+
+TEST_F(FrameGraphTest, WriteResourceReadAsAttachment) {
+
+    // this check s that using a resources as a read-only attachment doesn't set the
+    // discard flag on that resource. i.e. that the mere fact of reading a resource as an
+    // attachment doesn't make it invalid.
+    // see: https://github.com/google/filament/issues/5005
+
+    struct PassData {
+        FrameGraphId<FrameGraphTexture> input;
+    };
+    auto& writePass = fg.addPass<PassData>("Write Pass", [&](FrameGraph::Builder& builder, auto& data) {
+                data.input = builder.create<FrameGraphTexture>("Output buffer", {.width=16, .height=32});
+                data.input = builder.write(data.input, FrameGraphTexture::Usage::COLOR_ATTACHMENT);
+                builder.declareRenderPass("Output target", { .attachments = {
+                        .color = { data.input }
+                }});
+                builder.sideEffect();
+            },
+            [=](FrameGraphResources const& resources, auto const&, backend::DriverApi&) {
+                auto rt = resources.getRenderPassInfo();
+                EXPECT_EQ(rt.params.flags.discardStart, TargetBufferFlags::COLOR0);
+                EXPECT_EQ(rt.params.flags.discardEnd, TargetBufferFlags::NONE);
+            });
+
+    fg.addPass<PassData>("Read Pass", [&](FrameGraph::Builder& builder, auto& data) {
+                data.input = builder.read(writePass->input, FrameGraphTexture::Usage::COLOR_ATTACHMENT);
+                builder.declareRenderPass("Input target", { .attachments = {
+                        .color = { data.input }
+                }});
+                builder.sideEffect();
+            },
+            [=](FrameGraphResources const& resources, auto const&, backend::DriverApi&) {
+                auto rt = resources.getRenderPassInfo();
+                EXPECT_EQ(rt.params.flags.discardStart, TargetBufferFlags::NONE);
+                EXPECT_EQ(rt.params.flags.discardEnd, TargetBufferFlags::NONE);
+            });
+
+    fg.addPass<PassData>("Sample Pass", [&](FrameGraph::Builder& builder, auto& data) {
+                data.input = builder.sample(writePass->input);
+                builder.sideEffect();
+            },
+            [=](FrameGraphResources const& resources, auto const&, backend::DriverApi&) {
+            });
+
+
+    EXPECT_TRUE(fg.isAcyclic());
+
+    fg.compile();
+
+    fg.execute(driverApi);
+}