Fix #133565: Single value nodes wrongly return an image

In certain setups, nodes whose inputs are single value and whose outputs
are expected to be single value wrongly return an image. That's because
they wrongly join a pixel operation that operates on images.

To fix this, we split pixel operations by their value types. Single
value sub-trees get compiled into their own pixel operation and none
single value sub-trees get compiled into their own pixel operation.

This might unfortunately break up a long chain of pixel operations if
one of them was single value. This is solvable, but will require more
time and research, so we need to fix the bug first then look into it.

Pull Request: https://projects.blender.org/blender/blender/pulls/133701

source/blender/compositor/COM_compile_state.hh

@@ -4,6 +4,8 @@
 
 #pragma once
 
+#include <optional>
+
 #include "BLI_map.hh"
 
 #include "NOD_derived_node_tree.hh"
@@ -49,29 +51,33 @@ using namespace nodes::derived_node_tree_types;
  * |            |
  * '------------'
  *
- * Second, it stores the pixel compile unit as well as its domain. One should first go over the
+ * Second, it stores the pixel compile unit, whether is operates on single values, and its domain
- * discussion in COM_evaluator.hh for a high level description of the mechanism of the compile
+ * if it was not operating on single values. One should first go over the discussion in
- * unit. The one important detail in this class is the should_compile_pixel_compile_unit method,
+ * COM_evaluator.hh for a high level description of the mechanism of the compile unit. The one
- * which implements the criteria of whether the compile unit should be compiled given the node
+ * important detail in this class is the should_compile_pixel_compile_unit method, which implements
- * currently being processed as an argument. Those criteria are described as follows. If the
+ * the criteria of whether the compile unit should be compiled given the node currently being
- * compile unit is empty as is the case when processing nodes 1, 2, and 3, then it plainly
+ * processed as an argument. Those criteria are described as follows. If the compile unit is empty
- * shouldn't be compiled. If the given node is not a pixel node, then it can't be added to the
+ * as is the case when processing nodes 1, 2, and 3, then it plainly shouldn't be compiled. If the
- * compile unit and the unit is considered complete and should be compiled, as is the case when
+ * given node is not a pixel node, then it can't be added to the compile unit and the unit is
- * processing node 6. If the computed domain of the given node is not compatible with the domain of
+ * considered complete and should be compiled, as is the case when processing node 6. If the
- * the compiled unit, then it can't be added to the unit and the unit is considered complete and
+ * compile unit operates on single values and the given node operates on non-single values or vice
- * should be compiled, as is the case when processing node 5, more on this in the next section.
+ * versa, then it can't be added to the compile unit and the unit is considered complete and should
- * Otherwise, the given node is compatible with the compile unit and can be added to it, so the
+ * be compiled, more on that in the next section. If the computed domain of the given node is not
- * unit shouldn't be compiled just yet, as is the case when processing node 4.
+ * compatible with the domain of the compiled unit, then it can't be added to the unit and the unit
+ * is considered complete and should be compiled, as is the case when processing node 5, more on
+ * this in the next section. Otherwise, the given node is compatible with the compile unit and can
+ * be added to it, so the unit shouldn't be compiled just yet, as is the case when processing
+ * node 4.
  *
- * Special attention should be given to the aforementioned domain compatibility criterion. One
+ * Special attention should be given to the aforementioned single value and domain compatibility
- * should first go over the discussion in COM_domain.hh for more information on domains. When a
+ * criterion. One should first go over the discussion in COM_domain.hh for more information on
- * compile unit gets eventually compiled to a pixel operation, that operation will have a certain
+ * domains. When a compile unit gets eventually compiled to a pixel operation, that operation will
- * operation domain, and any node that gets added to the compile unit should itself have a computed
+ * have a certain operation domain, and any node that gets added to the compile unit should itself
- * node domain that is compatible with that operation domain, otherwise, had the node been compiled
+ * have a computed node domain that is compatible with that operation domain, otherwise, had the
- * into its own operation separately, the result would have been be different. For instance,
+ * node been compiled into its own operation separately, the result would have been be different.
- * consider the above node tree where node 1 outputs a 100x100 result, node 2 outputs a 50x50
+ * For instance, consider the above node tree where node 1 outputs a 100x100 result, node 2 outputs
- * result, the first input in node 3 has the highest domain priority, and the second input in node
+ * a 50x50 result, the first input in node 3 has the highest domain priority, and the second input
- * 5 has the highest domain priority. In this case, pixel operation 1 will output a 100x100
+ * in node 5 has the highest domain priority. In this case, pixel operation 1 will output a 100x100
  * result, and pixel operation 2 will output a 50x50 result, because that's the computed operation
  * domain for each of them. So node 6 will get a 50x50 result. Now consider the same node tree, but
  * where all three nodes 3, 4, and 5 were compiled into a single pixel operation as shown the node
@@ -94,15 +100,23 @@ using namespace nodes::derived_node_tree_types;
  * |            |
  * '------------'
  *
- * To check for the domain compatibility between the compile unit and the node being processed, the
+ * Similarly, all nodes in the compile unit should either be operating on single values or not.
- * domain of the compile unit is assumed to be the domain of the first node whose computed domain
+ * Otherwise, assuming a node operates on single values and its output is used in 1) a non-single
- * is not an identity domain. Identity domains corresponds to single value results, so those are
+ * value pixel operation and 2) another node that expects single values, if that node was added to
- * always compatible with any domain. The domain of the compile unit is computed and set in the
+ * the pixel operation, its output will be non-single value, while it would have been a single
- * add_node_to_pixel_compile_unit method. When processing a node, the computed domain of node is
+ * value if it was not added to the pixel operation.
- * compared to the compile unit domain in the should_compile_pixel_compile_unit method, noting that
+ *
- * identity domains are always compatible. Node domains are computed in the
+ * To check for the single value type and domain compatibility between the compile unit and the
- * compute_pixel_node_domain method, which is analogous to Operation::compute_domain for nodes
+ * node being processed, the single value type and the domain of the compile unit is assumed to be
- * that are not yet compiled. */
+ * the single value type and the domain of the first node added to the compile unit, noting that
+ * the domain is optional, since it is not used if the compile unit is a single value one. The
+ * single value type and the domain of the compile unit are computed and set in the
+ * add_node_to_pixel_compile_unit method. When processing a node, the computed single value type
+ * and the computed domain of node are compared to the compile unit single value type and domain in
+ * the should_compile_pixel_compile_unit method. Node single value types and domains are computed
+ * in the is_pixel_node_single_value and compute_pixel_node_domain methods respectively, the latter
+ * of which is analogous to the Operation::compute_domain method for nodes that are not yet
+ * compiled. */
 class CompileState {
  private:
   /* A reference to the node execution schedule that is being compiled. */
@@ -118,8 +132,12 @@ class CompileState {
    * be compiled together into a pixel operation. See the discussion in COM_evaluator.hh for more
    * information. */
   PixelCompileUnit pixel_compile_unit_;
-  /* The domain of the pixel compile unit. */
+  /* Stores whether the current pixel compile unit operates on single values. Only initialized when
-  Domain pixel_compile_unit_domain_ = Domain::identity();
+   * the pixel compile unit is not empty. */
+  bool is_pixel_compile_unit_single_value_;
+  /* The domain of the pixel compile unit if it was not a single value. Only initialized when the
+   * pixel compile unit is not empty and is not a single value. */
+  std::optional<Domain> pixel_compile_unit_domain_;
 
  public:
   /* Construct a compile state from the node execution schedule being compiled. */
@@ -148,18 +166,15 @@ class CompileState {
   /* Get a reference to the pixel compile unit. */
   PixelCompileUnit &get_pixel_compile_unit();
 
+  /* Returns true if the pixel compile unit operates on single values. */
+  bool is_pixel_compile_unit_single_value();
+
   /* Clear the compile unit. This should be called once the compile unit is compiled to ready it to
    * track the next potential compile unit. */
   void reset_pixel_compile_unit();
 
   /* Determines if the compile unit should be compiled based on a number of criteria give the node
-   * currently being processed. Those criteria are as follows:
+   * currently being processed. See the class description for a description of the method. */
-   * - If compile unit is empty, then it can't and shouldn't be compiled.
-   * - If the given node is not a pixel node, then it can't be added to the compile unit
-   *   and the unit is considered complete and should be compiled.
-   * - If the computed domain of the given node is not compatible with the domain of the compile
-   *   unit, then it can't be added to it and the unit is considered complete and should be
-   *   compiled. */
   bool should_compile_pixel_compile_unit(DNode node);
 
   /* Computes the number of pixel operation outputs that will be added for this node in the current
@@ -168,6 +183,11 @@ class CompileState {
   int compute_pixel_node_operation_outputs_count(DNode node);
 
  private:
+  /* Determines if the given pixel node operates on single values or not. The node operates on
+   * single values if all its inputs are single values, and consequently will also output single
+   * values. */
+  bool is_pixel_node_single_value(DNode node);
+
   /* Compute the node domain of the given pixel node. This is analogous to the
    * Operation::compute_domain method, except it is computed from the node itself as opposed to a
    * compiled operation. See the discussion in COM_domain.hh for more information. */

source/blender/compositor/COM_pixel_operation.hh

@@ -94,12 +94,6 @@ class PixelOperation : public Operation {
  public:
   PixelOperation(Context &context, PixelCompileUnit &compile_unit, const Schedule &schedule);
 
-  /* Create one of the concrete subclasses based on the context. Deleting the operation is the
-   * caller's responsibility. */
-  static PixelOperation *create_operation(Context &context,
-                                          PixelCompileUnit &compile_unit,
-                                          const Schedule &schedule);
-
   /* Returns the maximum number of outputs that the PixelOperation can have. Pixel compile units
    * need to be split into smaller units if the numbers of outputs they have is more than the
    * number returned by this method. */

source/blender/compositor/intern/compile_state.cc

@@ -60,10 +60,16 @@ void CompileState::add_node_to_pixel_compile_unit(DNode node)
 {
   pixel_compile_unit_.add_new(node);
 
-  /* If the domain of the pixel compile unit is not yet determined or was determined to be
+  /* If this is the first node in the compile unit, then we should initialize the single value
-   * an identity domain, update it to be the computed domain of the node. */
+   * type, as well as the domain in case the node was not single value. */
-  if (pixel_compile_unit_domain_ == Domain::identity()) {
+  const bool is_first_node_in_operation = pixel_compile_unit_.size() == 1;
-    pixel_compile_unit_domain_ = compute_pixel_node_domain(node);
+  if (is_first_node_in_operation) {
+    is_pixel_compile_unit_single_value_ = this->is_pixel_node_single_value(node);
+
+    /* If the node was not a single value, compute and initialize the domain. */
+    if (!is_pixel_compile_unit_single_value_) {
+      pixel_compile_unit_domain_ = this->compute_pixel_node_domain(node);
+    }
   }
 }
 
@@ -72,10 +78,15 @@ PixelCompileUnit &CompileState::get_pixel_compile_unit()
   return pixel_compile_unit_;
 }
 
+bool CompileState::is_pixel_compile_unit_single_value()
+{
+  return is_pixel_compile_unit_single_value_;
+}
+
 void CompileState::reset_pixel_compile_unit()
 {
   pixel_compile_unit_.clear();
-  pixel_compile_unit_domain_ = Domain::identity();
+  pixel_compile_unit_domain_.reset();
 }
 
 bool CompileState::should_compile_pixel_compile_unit(DNode node)
@@ -91,16 +102,22 @@ bool CompileState::should_compile_pixel_compile_unit(DNode node)
     return true;
   }
 
-  /* If the computed domain of the node doesn't matches the domain of the pixel compile unit, then
+  /* If the compile unit is single value and the given node is not or vice versa, then it can't be
-   * it can't be added to the pixel compile unit and the pixel compile unit is considered
+   * added to the pixel compile unit and the pixel compile unit is considered complete and should
-   * complete and should be compiled. Identity domains are an exception as they are always
+   * be compiled. */
-   * compatible because they represents single values. */
+  if (is_pixel_compile_unit_single_value_ != this->is_pixel_node_single_value(node)) {
-  if (pixel_compile_unit_domain_ != Domain::identity() &&
-      pixel_compile_unit_domain_ != compute_pixel_node_domain(node))
-  {
     return true;
   }
 
+  /* For non single value compile units, if the computed domain of the node doesn't matches the
+   * domain of the pixel compile unit, then it can't be added to the pixel compile unit and the
+   * pixel compile unit is considered complete and should be compiled. */
+  if (!is_pixel_compile_unit_single_value_) {
+    if (pixel_compile_unit_domain_.value() != this->compute_pixel_node_domain(node)) {
+      return true;
+    }
+  }
+
   /* Otherwise, the node is compatible and can be added to the compile unit and it shouldn't be
    * compiled just yet. */
   return false;
@@ -132,6 +149,37 @@ int CompileState::compute_pixel_node_operation_outputs_count(DNode node)
   return outputs_count;
 }
 
+bool CompileState::is_pixel_node_single_value(DNode node)
+{
+  /* The pixel node is single value when all of its inputs are single values. */
+  for (const bNodeSocket *input : node->input_sockets()) {
+    const DInputSocket dinput{node.context(), input};
+
+    /* Get the output linked to the input. If it is null, that means the input is unlinked, and is
+     * thus single value. */
+    const DOutputSocket output = get_output_linked_to_input(dinput);
+    if (!output) {
+      continue;
+    }
+
+    /* If the output belongs to a node that is part of the pixel compile unit and that compile unit
+     * is not single value, then the node is not single value. */
+    if (pixel_compile_unit_.contains(output.node())) {
+      if (is_pixel_compile_unit_single_value_) {
+        continue;
+      }
+      return false;
+    }
+
+    const Result &result = get_result_from_output_socket(output);
+    if (!result.is_single_value()) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 Domain CompileState::compute_pixel_node_domain(DNode node)
 {
   /* Default to an identity domain in case no domain input was found, most likely because all
@@ -156,15 +204,10 @@ Domain CompileState::compute_pixel_node_domain(DNode node)
     /* If the output belongs to a node that is part of the pixel compile unit, then the domain of
      * the input is the domain of the compile unit itself. */
     if (pixel_compile_unit_.contains(output.node())) {
-      /* Single value inputs can't be domain inputs. */
-      if (pixel_compile_unit_domain_.size == int2(1)) {
-        continue;
-      }
-
       /* Notice that the lower the domain priority value is, the higher the priority is, hence the
        * less than comparison. */
       if (input_descriptor.domain_priority < current_domain_priority) {
-        node_domain = pixel_compile_unit_domain_;
+        node_domain = pixel_compile_unit_domain_.value();
         current_domain_priority = input_descriptor.domain_priority;
       }
       continue;

source/blender/compositor/intern/evaluator.cc

@@ -12,6 +12,7 @@
 #include "COM_context.hh"
 #include "COM_evaluator.hh"
 #include "COM_input_single_value_operation.hh"
+#include "COM_multi_function_procedure_operation.hh"
 #include "COM_node_operation.hh"
 #include "COM_operation.hh"
 #include "COM_result.hh"
@@ -159,6 +160,22 @@ void Evaluator::map_node_operation_inputs_to_their_results(DNode node,
   }
 }
 
+/* Create one of the concrete subclasses of the PixelOperation based on the context and compile
+ * state. Deleting the operation is the caller's responsibility. */
+PixelOperation *create_pixel_operation(Context &context, CompileState &compile_state)
+{
+  const Schedule &schedule = compile_state.get_schedule();
+  PixelCompileUnit &compile_unit = compile_state.get_pixel_compile_unit();
+
+  /* Use multi-function procedure to execute the pixel compile unit for CPU contexts or if the
+   * compile unit is single value and would thus be more efficient to execute on the CPU. */
+  if (!context.use_gpu() || compile_state.is_pixel_compile_unit_single_value()) {
+    return new MultiFunctionProcedureOperation(context, compile_unit, schedule);
+  }
+
+  return new ShaderOperation(context, compile_unit, schedule);
+}
+
 void Evaluator::compile_and_evaluate_pixel_compile_unit(CompileState &compile_state)
 {
   PixelCompileUnit &compile_unit = compile_state.get_pixel_compile_unit();
@@ -199,8 +216,7 @@ void Evaluator::compile_and_evaluate_pixel_compile_unit(CompileState &compile_st
     return;
   }
 
-  const Schedule &schedule = compile_state.get_schedule();
+  PixelOperation *operation = create_pixel_operation(context_, compile_state);
-  PixelOperation *operation = PixelOperation::create_operation(context_, compile_unit, schedule);
 
   for (DNode node : compile_unit) {
     compile_state.map_node_to_pixel_operation(node, operation);

source/blender/compositor/intern/pixel_operation.cc

@@ -31,17 +31,6 @@ PixelOperation::PixelOperation(Context &context,
 {
 }
 
-PixelOperation *PixelOperation::create_operation(Context &context,
-                                                 PixelCompileUnit &compile_unit,
-                                                 const Schedule &schedule)
-{
-  if (context.use_gpu()) {
-    return new ShaderOperation(context, compile_unit, schedule);
-  }
-
-  return new MultiFunctionProcedureOperation(context, compile_unit, schedule);
-}
-
 int PixelOperation::maximum_number_of_outputs(Context &context)
 {
   if (context.use_gpu()) {