From acea32aae8fd2735ba317d972beea66b153edd0c Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 18 Jan 2019 13:05:00 -0800 Subject: [PATCH] more validation in the framegraph - validate that a pass actually uses a resource when requesting the concrete (Driver) handle. - introduce a "blit" usage for a resource, which indicates that a resource will be used as the source of a blit operation. This is unfortunatelly needed because blit operations work on rendertargets, not textures for the source. --- filament/src/PostProcessManager.cpp | 7 +- filament/src/fg/FrameGraph.cpp | 92 ++++++++++++++++------ filament/src/fg/FrameGraph.h | 3 + filament/test/filament_framegraph_test.cpp | 4 +- 4 files changed, 77 insertions(+), 29 deletions(-) diff --git a/filament/src/PostProcessManager.cpp b/filament/src/PostProcessManager.cpp index 7d8d0b4b39..945e45715d 100644 --- a/filament/src/PostProcessManager.cpp +++ b/filament/src/PostProcessManager.cpp @@ -199,7 +199,7 @@ FrameGraphResource PostProcessManager::msaa(FrameGraph& fg, auto& ppMSAA = fg.addPass("msaa", [&](FrameGraph::Builder& builder, PostProcessMSAA& data) { auto const* inputDesc = fg.getDescriptor(input); - data.input = builder.read(input); + data.input = builder.blit(input); FrameGraphResource::Descriptor outputDesc{ .width = inputDesc->width, @@ -208,8 +208,7 @@ FrameGraphResource PostProcessManager::msaa(FrameGraph& fg, }; data.output = builder.write(builder.createResource("msaa output", outputDesc)); }, - [=]( - FrameGraphPassResources const& resources, + [=](FrameGraphPassResources const& resources, PostProcessMSAA const& data, DriverApi& driver) { auto in = resources.getRenderTarget(data.input); auto out = resources.getRenderTarget(data.output); @@ -333,7 +332,7 @@ FrameGraphResource PostProcessManager::dynamicScaling(FrameGraph& fg, auto& ppScaling = fg.addPass("scaling", [&](FrameGraph::Builder& builder, PostProcessScaling& data) { auto* inputDesc = fg.getDescriptor(input); - data.input = builder.read(input); + data.input = builder.blit(input); FrameGraphResource::Descriptor outputDesc{ .width = inputDesc->width, diff --git a/filament/src/fg/FrameGraph.cpp b/filament/src/fg/FrameGraph.cpp index 185e4095c7..2d65025fd0 100644 --- a/filament/src/fg/FrameGraph.cpp +++ b/filament/src/fg/FrameGraph.cpp @@ -140,6 +140,12 @@ struct PassNode { return r; } + bool isReadingFrom(FrameGraphResource resource) const noexcept { + auto pos = std::find_if(reads.begin(), reads.end(), + [resource](FrameGraphResource cur) { return resource.index == cur.index; }); + return (pos != reads.end()); + } + FrameGraphResource write(ResourceNode& resource) { // don't allow multiple writes of the same resource -- it's just redundant. auto pos = std::find_if(writes.begin(), writes.end(), @@ -172,6 +178,12 @@ struct PassNode { return r; } + bool isWritingTo(FrameGraphResource resource) const noexcept { + auto pos = std::find_if(writes.begin(), writes.end(), + [resource](FrameGraphResource cur) { return resource.index == cur.index; }); + return (pos != writes.end()); + } + // constants const char* const name; // our name const uint32_t id; // a unique id (only for debugging) @@ -180,6 +192,9 @@ struct PassNode { // set by the builder ResourceList reads; // resources we're reading from ResourceList writes; // resources we're writing to + + // TODO: we need to store discardFlags per resource written + bool hasSideEffect = false; // whether this pass has side effects // computed during compile() @@ -249,29 +264,37 @@ void Resource::create(DriverApi& driver) noexcept { if (readerCount) { assert(readFlags); - if (readFlags & FrameGraph::Builder::COLOR) { - textures[0] = driver.createTexture(desc.type, desc.levels, - desc.format, 1, - desc.width, desc.height, desc.depth, - TextureUsage::COLOR_ATTACHMENT); - } - if (readFlags & FrameGraph::Builder::DEPTH) { - textures[1] = driver.createTexture(desc.type, desc.levels, - TextureFormat::DEPTH24, 1, - desc.width, desc.height, desc.depth, - TextureUsage::DEPTH_ATTACHMENT); - } } + if (writerCount) { assert(writeFlags); - uint32_t attachments = 0; - if (writeFlags & FrameGraph::Builder::COLOR) { - attachments |= uint32_t(TargetBufferFlags::COLOR); - } - if (writeFlags & FrameGraph::Builder::DEPTH) { - attachments |= TargetBufferFlags::DEPTH; - } + } + if (readFlags & FrameGraph::Builder::COLOR) { + textures[0] = driver.createTexture(desc.type, desc.levels, + desc.format, 1, + desc.width, desc.height, desc.depth, + TextureUsage::COLOR_ATTACHMENT); + } + if (readFlags & FrameGraph::Builder::DEPTH) { + textures[1] = driver.createTexture(desc.type, desc.levels, + TextureFormat::DEPTH24, 1, + desc.width, desc.height, desc.depth, + TextureUsage::DEPTH_ATTACHMENT); + } + + // Note: if the resource is a source of a blit, it needs a rendertarget (because that's how + // blits work) -- in that case, the resource would have been declared with Builder::blit() + // access, and its write flags here would be set. + + uint32_t attachments = 0; + if (writeFlags & FrameGraph::Builder::COLOR) { + attachments |= uint32_t(TargetBufferFlags::COLOR); + } + if (writeFlags & FrameGraph::Builder::DEPTH) { + attachments |= TargetBufferFlags::DEPTH; + } + if (attachments) { target.target = driver.createRenderTarget(TargetBufferFlags(attachments), desc.width, desc.height, desc.samples, desc.format, { textures[0] }, { textures[1] }, {}); @@ -332,6 +355,18 @@ FrameGraphResource FrameGraph::Builder::read(FrameGraphResource const& input, RW return mPass.read(*resource); } +FrameGraphResource +FrameGraph::Builder::blit(FrameGraphResource const& input, FrameGraph::Builder::RWFlags readFlags) { + ResourceNode* resource = mFrameGraph.getResource(input); + if (!resource) { + return {}; + } + resource->readFlags |= readFlags; + // resource used in a blit, it needs a rendertarget, so we set its write flags. + resource->writeFlags |= readFlags; + return mPass.read(*resource); +} + FrameGraphResource FrameGraph::Builder::write(FrameGraphResource const& output, RWFlags writeFlags) { ResourceNode* resource = mFrameGraph.getResource(output); if (!resource) { @@ -354,11 +389,16 @@ FrameGraphPassResources::FrameGraphPassResources(FrameGraph& fg, fg::PassNode co Handle FrameGraphPassResources::getTexture( FrameGraphResource r, TextureUsage attachment) const noexcept { - // TODO: we should check that this FrameGraphResource is indeed used by this pass - (void)mPass; // suppress unused warning + Resource const* const pResource = mFrameGraph.mResourceNodes[r.index].resource; assert(pResource); + // check that this FrameGraphResource is indeed used by this pass + ASSERT_POSTCONDITION_NON_FATAL(mPass.isReadingFrom(r), + "Pass \"%s\" doesn't declare reads to resource \"%s\" -- expect graphic corruptions", + mPass.name, pResource->name); + + const char* requested = "unknown"; Handle h; switch (attachment) { @@ -394,10 +434,16 @@ Handle FrameGraphPassResources::getTexture( FrameGraphPassResources::RenderTarget const& FrameGraphPassResources::getRenderTarget(FrameGraphResource r) const noexcept { - // TODO: we should check that this FrameGraphResource is indeed used by this pass - (void)mPass; // suppress unused warning Resource const* const pResource = mFrameGraph.mResourceNodes[r.index].resource; assert(pResource); + + // We need to check the resource in read or written by this pass (as opposed to written only), + // because a render target is needed for blit operations, which might be what the caller + // is doing. + ASSERT_POSTCONDITION_NON_FATAL(mPass.isWritingTo(r) || mPass.isReadingFrom(r), + "Pass \"%s\" doesn't declare writes to resource \"%s\" -- expect graphic corruptions", + mPass.name, pResource->name); + assert(pResource->target.target); if (pResource->imported) { diff --git a/filament/src/fg/FrameGraph.h b/filament/src/fg/FrameGraph.h index ba73f431a3..d42b538e86 100644 --- a/filament/src/fg/FrameGraph.h +++ b/filament/src/fg/FrameGraph.h @@ -70,6 +70,9 @@ public: // Read from a resource (i.e. add a reference to that resource) FrameGraphResource read(FrameGraphResource const& input, RWFlags readFlags = COLOR); + // The resource will be used as a source of a blit() + FrameGraphResource blit(FrameGraphResource const& input, RWFlags readFlags = COLOR); + // Write to a resource (i.e. add a reference to the pass that's doing the writing)) // Writing to a resource makes its handle invalid. // Writing to an imported resources adds a side-effect (see sideEffect() below). diff --git a/filament/test/filament_framegraph_test.cpp b/filament/test/filament_framegraph_test.cpp index bcca832651..230eca0ec0 100644 --- a/filament/test/filament_framegraph_test.cpp +++ b/filament/test/filament_framegraph_test.cpp @@ -51,7 +51,7 @@ TEST(FrameGraphTest, SimpleRenderPass) { RenderPassData const& data, driver::DriverApi& driver) { renderPassExecuted = true; - EXPECT_TRUE(resources.getTexture(data.output)); + EXPECT_TRUE(resources.getRenderTarget(data.output).target); }); @@ -115,7 +115,7 @@ TEST(FrameGraphTest, SimpleRenderAndPostProcessPasses) { RenderPassData const& data, driver::DriverApi& driver) { renderPassExecuted = true; - EXPECT_TRUE(resources.getTexture(data.output)); + EXPECT_TRUE(resources.getRenderTarget(data.output).target); });