From b03e9be22d2df811db7d4d6acf1d7fe9f2a79ef4 Mon Sep 17 00:00:00 2001 From: Omar Emara Date: Tue, 4 Mar 2025 15:05:41 +0200 Subject: [PATCH] Refactor: Compositor: Assert for unneeded allocations This patch refactors how the compositor deals with outputs that are allocated but not needed. Previously we allowed such allocations and implicitly release them right after operations are computed. This is a weak design, so we now require developers to only allocate outputs that are actually needed and assert otherwise. The release mechanism is therefore removed. --- source/blender/compositor/COM_operation.hh | 6 ------ source/blender/compositor/COM_result.hh | 13 +------------ source/blender/compositor/intern/operation.cc | 11 ----------- source/blender/compositor/intern/result.cc | 12 +++++------- 4 files changed, 6 insertions(+), 36 deletions(-) diff --git a/source/blender/compositor/COM_operation.hh b/source/blender/compositor/COM_operation.hh index b4a67b79c62..9ecd6c4a002 100644 --- a/source/blender/compositor/COM_operation.hh +++ b/source/blender/compositor/COM_operation.hh @@ -174,12 +174,6 @@ class Operation { * evaluation of the operation to declare that the results are no longer needed by this * operation. */ void release_inputs(); - - /* Release the results that were allocated in the execute method but are not actually needed. - * This can be the case if the execute method allocated a dummy texture for an unneeded result, - * see the description of Result::allocate_texture() for more information. This is called after - * the evaluation of the operation. */ - void release_unneeded_results(); }; } // namespace blender::compositor diff --git a/source/blender/compositor/COM_result.hh b/source/blender/compositor/COM_result.hh index 11ecacb45fa..783298502ea 100644 --- a/source/blender/compositor/COM_result.hh +++ b/source/blender/compositor/COM_result.hh @@ -224,18 +224,7 @@ class Result { * same evaluation. * * If the context of the result uses GPU, then GPU allocation will be done, otherwise, CPU - * allocation will be done. - * - * If the result should not be computed, that is, should_compute() returns false, yet this method - * is called, that means the result is only being allocated because the shader that computes it - * also computes another result that is actually needed, and shaders needs to have a texture - * bound to all their images units for a correct invocation, even if some of those textures are - * not needed and will eventually be discarded. In that case, since allocating the full texture - * is not needed, allocate_single_value() is called instead and the reference count is set to 1. - * This essentially allocates a dummy 1x1 texture, which works because out of bound shader writes - * to images are safe. Since this result is not referenced by any other operation, it should be - * manually released after the operation is evaluated, which is implemented by calling the - * Operation::release_unneeded_results() method. */ + * allocation will be done. */ void allocate_texture(Domain domain, bool from_pool = true); /* Declare the result to be a single value result, allocate a texture of an appropriate type with diff --git a/source/blender/compositor/intern/operation.cc b/source/blender/compositor/intern/operation.cc index 8df3c7b70e8..fc819cceab0 100644 --- a/source/blender/compositor/intern/operation.cc +++ b/source/blender/compositor/intern/operation.cc @@ -35,8 +35,6 @@ void Operation::evaluate() release_inputs(); - release_unneeded_results(); - context().evaluate_operation_post(); } @@ -164,15 +162,6 @@ InputDescriptor &Operation::get_input_descriptor(StringRef identifier) return input_descriptors_.lookup(identifier); } -void Operation::release_unneeded_results() -{ - for (Result &result : results_.values()) { - if (!result.should_compute() && result.is_allocated()) { - result.release(); - } - } -} - Context &Operation::context() const { return context_; diff --git a/source/blender/compositor/intern/result.cc b/source/blender/compositor/intern/result.cc index c9ad69325d5..6251c6f46c8 100644 --- a/source/blender/compositor/intern/result.cc +++ b/source/blender/compositor/intern/result.cc @@ -261,13 +261,8 @@ eGPUTextureFormat Result::get_gpu_texture_format() const void Result::allocate_texture(Domain domain, bool from_pool) { - /* The result is not actually needed, so allocate a dummy single value texture instead. See the - * method description for more information. */ - if (!should_compute()) { - allocate_single_value(); - increment_reference_count(); - return; - } + /* Make sure we are not allocating a result that should not be computed. */ + BLI_assert(this->should_compute()); is_single_value_ = false; this->allocate_data(domain.size, from_pool); @@ -276,6 +271,9 @@ void Result::allocate_texture(Domain domain, bool from_pool) void Result::allocate_single_value() { + /* Make sure we are not allocating a result that should not be computed. */ + BLI_assert(this->should_compute()); + /* Single values are stored in 1x1 image as well as the single value members. Further, they * are always allocated from the pool. */ is_single_value_ = true;