From fb6b75951359c48876edf44c6a36348111d697f2 Mon Sep 17 00:00:00 2001 From: Sergey Sharybin Date: Fri, 17 May 2024 10:20:02 +0200 Subject: [PATCH] Cleanup: Avoid reference of RenderLayer and RenderPass in multilayer operation This is avoids reference to data which can potentially be freed from the main thread while the compositor job is running. There is still some direct access to RenderResult and access to its layers and passes in the operation implementation, but is is all internal and will be worked on later. The purpose of this patch is to avoid unsafe pointers in the API of the operation. Should be no functional changes. Ref #118337, #121761 --- .../compositor/nodes/COM_CryptomatteNode.cc | 3 +- .../blender/compositor/nodes/COM_ImageNode.cc | 19 +++++------- .../blender/compositor/nodes/COM_ImageNode.h | 3 +- .../COM_MultilayerImageOperation.cc | 29 +++++++++---------- .../operations/COM_MultilayerImageOperation.h | 27 ++++++++--------- 5 files changed, 36 insertions(+), 45 deletions(-) diff --git a/source/blender/compositor/nodes/COM_CryptomatteNode.cc b/source/blender/compositor/nodes/COM_CryptomatteNode.cc index 7787609eabd..5470f0c19ad 100644 --- a/source/blender/compositor/nodes/COM_CryptomatteNode.cc +++ b/source/blender/compositor/nodes/COM_CryptomatteNode.cc @@ -168,12 +168,13 @@ void CryptomatteNode::input_operations_from_image_source( LISTBASE_FOREACH (RenderPass *, render_pass, &render_layer->passes) { const std::string combined_name = combined_layer_pass_name(render_layer, render_pass); if (combined_name != prefix && blender::StringRef(combined_name).startswith(prefix)) { - MultilayerColorOperation *op = new MultilayerColorOperation(render_layer, render_pass); + MultilayerColorOperation *op = new MultilayerColorOperation(); iuser->layer = layer_index; op->set_image(image); op->set_image_user(*iuser); op->set_framenumber(context.get_framenumber()); op->set_view_name(context.get_view_name()); + op->set_pass_name(render_pass->name); r_input_operations.append(op); } } diff --git a/source/blender/compositor/nodes/COM_ImageNode.cc b/source/blender/compositor/nodes/COM_ImageNode.cc index 9d3cb077b3f..2a8a7d91823 100644 --- a/source/blender/compositor/nodes/COM_ImageNode.cc +++ b/source/blender/compositor/nodes/COM_ImageNode.cc @@ -20,8 +20,7 @@ ImageNode::ImageNode(bNode *editor_node) : Node(editor_node) } NodeOperation *ImageNode::do_multilayer_check(NodeConverter &converter, const CompositorContext &context, - RenderLayer *render_layer, - RenderPass *render_pass, + const char *pass_name, Image *image, ImageUser *user, int framenumber, @@ -32,13 +31,13 @@ NodeOperation *ImageNode::do_multilayer_check(NodeConverter &converter, MultilayerBaseOperation *operation = nullptr; switch (datatype) { case DataType::Value: - operation = new MultilayerValueOperation(render_layer, render_pass); + operation = new MultilayerValueOperation(); break; case DataType::Vector: - operation = new MultilayerVectorOperation(render_layer, render_pass); + operation = new MultilayerVectorOperation(); break; case DataType::Color: - operation = new MultilayerColorOperation(render_layer, render_pass); + operation = new MultilayerColorOperation(); break; default: break; @@ -47,6 +46,7 @@ NodeOperation *ImageNode::do_multilayer_check(NodeConverter &converter, operation->set_image_user(*user); operation->set_framenumber(framenumber); operation->set_view_name(context.get_view_name()); + operation->set_pass_name(pass_name); converter.add_operation(operation); converter.map_output_socket(output_socket, operation->get_output_socket()); @@ -94,8 +94,7 @@ void ImageNode::convert_to_operations(NodeConverter &converter, case 1: operation = do_multilayer_check(converter, context, - rl, - rpass, + rpass->name, image, imageuser, framenumber, @@ -107,8 +106,7 @@ void ImageNode::convert_to_operations(NodeConverter &converter, case 3: operation = do_multilayer_check(converter, context, - rl, - rpass, + rpass->name, image, imageuser, framenumber, @@ -118,8 +116,7 @@ void ImageNode::convert_to_operations(NodeConverter &converter, case 4: operation = do_multilayer_check(converter, context, - rl, - rpass, + rpass->name, image, imageuser, framenumber, diff --git a/source/blender/compositor/nodes/COM_ImageNode.h b/source/blender/compositor/nodes/COM_ImageNode.h index 32e9233785b..e4087e079ad 100644 --- a/source/blender/compositor/nodes/COM_ImageNode.h +++ b/source/blender/compositor/nodes/COM_ImageNode.h @@ -22,8 +22,7 @@ class ImageNode : public Node { private: NodeOperation *do_multilayer_check(NodeConverter &converter, const CompositorContext &context, - RenderLayer *render_layer, - RenderPass *render_pass, + const char *pass_name, Image *image, ImageUser *user, int framenumber, diff --git a/source/blender/compositor/operations/COM_MultilayerImageOperation.cc b/source/blender/compositor/operations/COM_MultilayerImageOperation.cc index f000d027c2f..142ae91d21a 100644 --- a/source/blender/compositor/operations/COM_MultilayerImageOperation.cc +++ b/source/blender/compositor/operations/COM_MultilayerImageOperation.cc @@ -10,14 +10,6 @@ namespace blender::compositor { -MultilayerBaseOperation::MultilayerBaseOperation(RenderLayer *render_layer, - RenderPass *render_pass) -{ - pass_id_ = BLI_findindex(&render_layer->passes, render_pass); - render_layer_ = render_layer; - render_pass_ = render_pass; -} - int MultilayerBaseOperation::get_view_index() const { if (BLI_listbase_count_at_most(&image_->rr->views, 2) <= 1) { @@ -43,8 +35,16 @@ int MultilayerBaseOperation::get_view_index() const ImBuf *MultilayerBaseOperation::get_im_buf() { + if (image_ == nullptr) { + return nullptr; + } + + const RenderLayer *render_layer = static_cast( + BLI_findlink(&image_->rr->layers, image_user_.layer)); + image_user_.view = get_view_index(); - image_user_.pass = pass_id_; + image_user_.pass = BLI_findstringindex( + &render_layer->passes, pass_name_.c_str(), offsetof(RenderPass, name)); if (BKE_image_multilayer_index(image_->rr, &image_user_)) { return BaseImageOperation::get_im_buf(); @@ -69,15 +69,12 @@ std::unique_ptr MultilayerColorOperation::get_meta_data() { BLI_assert(buffer_); MetaDataExtractCallbackData callback_data = {nullptr}; + /* TODO: Make access to the render result thread-safe. */ RenderResult *render_result = image_->rr; if (render_result && render_result->stamp_data) { - RenderLayer *render_layer = render_layer_; - RenderPass *render_pass = render_pass_; - std::string full_layer_name = - std::string(render_layer->name, - BLI_strnlen(render_layer->name, sizeof(render_layer->name))) + - "." + - std::string(render_pass->name, BLI_strnlen(render_pass->name, sizeof(render_pass->name))); + const RenderLayer *render_layer = static_cast( + BLI_findlink(&image_->rr->layers, image_user_.layer)); + std::string full_layer_name = std::string(render_layer->name) + "." + pass_name_; blender::StringRef cryptomatte_layer_name = blender::bke::cryptomatte::BKE_cryptomatte_extract_layer_name(full_layer_name); callback_data.set_cryptomatte_keys(cryptomatte_layer_name); diff --git a/source/blender/compositor/operations/COM_MultilayerImageOperation.h b/source/blender/compositor/operations/COM_MultilayerImageOperation.h index 16d821ce2ed..cba89fc397d 100644 --- a/source/blender/compositor/operations/COM_MultilayerImageOperation.h +++ b/source/blender/compositor/operations/COM_MultilayerImageOperation.h @@ -4,17 +4,15 @@ #pragma once +#include + #include "COM_ImageOperation.h" namespace blender::compositor { class MultilayerBaseOperation : public BaseImageOperation { - private: - int pass_id_; - protected: - RenderLayer *render_layer_; - RenderPass *render_pass_; + std::string pass_name_; /* Returns the image view to use for the current active view. */ int get_view_index() const; @@ -22,10 +20,12 @@ class MultilayerBaseOperation : public BaseImageOperation { ImBuf *get_im_buf() override; public: - /** - * Constructor - */ - MultilayerBaseOperation(RenderLayer *render_layer, RenderPass *render_pass); + MultilayerBaseOperation() = default; + + void set_pass_name(std::string pass_name) + { + pass_name_ = std::move(pass_name); + } void update_memory_buffer_partial(MemoryBuffer *output, const rcti &area, @@ -34,8 +34,7 @@ class MultilayerBaseOperation : public BaseImageOperation { class MultilayerColorOperation : public MultilayerBaseOperation { public: - MultilayerColorOperation(RenderLayer *render_layer, RenderPass *render_pass) - : MultilayerBaseOperation(render_layer, render_pass) + MultilayerColorOperation() { this->add_output_socket(DataType::Color); } @@ -44,8 +43,7 @@ class MultilayerColorOperation : public MultilayerBaseOperation { class MultilayerValueOperation : public MultilayerBaseOperation { public: - MultilayerValueOperation(RenderLayer *render_layer, RenderPass *render_pass) - : MultilayerBaseOperation(render_layer, render_pass) + MultilayerValueOperation() { this->add_output_socket(DataType::Value); } @@ -53,8 +51,7 @@ class MultilayerValueOperation : public MultilayerBaseOperation { class MultilayerVectorOperation : public MultilayerBaseOperation { public: - MultilayerVectorOperation(RenderLayer *render_layer, RenderPass *render_pass) - : MultilayerBaseOperation(render_layer, render_pass) + MultilayerVectorOperation() { this->add_output_socket(DataType::Vector); }