Skip to content

Commit

Permalink
fix potential framegraph textures use-after free
Browse files Browse the repository at this point in the history
make sure to unset all textures in the per-view sampler group after
they are used, because the resource could be destroyed after the
pass is finished

- unset the fog and ibl_specular after the color pass
- move that cleanup a bit earlier
- in the case of screen-space reflection the structure pass is
  set, but might not be used in the color pass, so we also need to
  unset it after the SSR pass and before any other passes.
  • Loading branch information
pixelflinger committed Aug 30, 2024
1 parent 2aa51db commit 2b620e6
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 8 deletions.
4 changes: 3 additions & 1 deletion filament/src/PerViewUniforms.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -459,10 +459,12 @@ void PerViewUniforms::bind(backend::DriverApi& driver) noexcept {

void PerViewUniforms::unbindSamplers() noexcept {
auto& samplerGroup = mSamplers;
samplerGroup.clearSampler(PerViewSib::SHADOW_MAP);
samplerGroup.clearSampler(PerViewSib::IBL_SPECULAR);
samplerGroup.clearSampler(PerViewSib::SSAO);
samplerGroup.clearSampler(PerViewSib::SSR);
samplerGroup.clearSampler(PerViewSib::STRUCTURE);
samplerGroup.clearSampler(PerViewSib::SHADOW_MAP);
samplerGroup.clearSampler(PerViewSib::FOG);
}

} // namespace filament
Expand Down
19 changes: 12 additions & 7 deletions filament/src/details/Renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1019,6 +1019,10 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
{ .width = svp.width, .height = svp.height });

if (UTILS_LIKELY(reflections)) {
fg.addTrivialSideEffectPass("SSR Cleanup", [&view](DriverApi& driver) {
view.getPerViewUniforms().prepareStructure({});
view.commitUniforms(driver);
});
// generate the mipchain
PostProcessManager::generateMipmapSSR(ppm, fg,
reflections, ssrConfig.reflection, false, ssrConfig);
Expand Down Expand Up @@ -1134,6 +1138,12 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
}
}

fg.addTrivialSideEffectPass("Finish Color Passes", [&view](DriverApi& driver) {
// Unbind SSAO sampler, b/c the FrameGraph will delete the texture at the end of the pass.
view.cleanupRenderPasses();
view.commitUniforms(driver);
});

if (colorGradingConfig.customResolve) {
assert_invariant(fg.getDescriptor(colorPassOutput.linearColor).samples <= 1);
// TODO: we have to "uncompress" (i.e. detonemap) the color buffer here because it's used
Expand Down Expand Up @@ -1172,16 +1182,11 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
// this is the output of the color pass / input to post processing,
// this is only used later for comparing it with the output after post-processing
FrameGraphId<FrameGraphTexture> const postProcessInput = colorGradingConfig.asSubpass ?
colorPassOutput.tonemappedColor :
colorPassOutput.linearColor;
colorPassOutput.tonemappedColor :
colorPassOutput.linearColor;

// input can change below
FrameGraphId<FrameGraphTexture> input = postProcessInput;
fg.addTrivialSideEffectPass("Finish Color Passes", [&view](DriverApi& driver) {
// Unbind SSAO sampler, b/c the FrameGraph will delete the texture at the end of the pass.
view.cleanupRenderPasses();
view.commitUniforms(driver);
});

// Resolve depth -- which might be needed because of TAA or DoF. This pass will be culled
// if the depth is not used below or if the depth is not MS (e.g. it could have been
Expand Down

0 comments on commit 2b620e6

Please sign in to comment.