blender
/
blender
136
404
Fork 586
Code Issues 6.2k Pull Requests 761 Projects 19 Wiki Activity

Render: Skip writing output if no Composite node exists #117129

Merged
Omar Emara merged 5 commits from OmarEmaraDev/blender:skip-no-composite-write into main 2024-01-24 12:00:59 +01:00
Conversation 11 Commits 5 Files Changed 2 +53 -10
Omar Emara commented 2024-01-15 12:55:13 +01:00
Member
Copy Link

This patch changes the render pipeline such that it doesn't write the
render result if the render pipeline is a compositor one whose node tree
does not have an output Composite node.

This is done to avoid double outputs when users use the File Output node
for saving the results instead. Additionally, it fixes crashes and empty
outputs when the node tree does not reference any render layers and has
no Composite output.

The render pipeline early fails if neither a File Output or a Composite
output node exists, so an output is guaranteed to always exist.

Fixes #115983.

This patch changes the render pipeline such that it doesn't write the render result if the render pipeline is a compositor one whose node tree does not have an output Composite node. This is done to avoid double outputs when users use the File Output node for saving the results instead. Additionally, it fixes crashes and empty outputs when the node tree does not reference any render layers and has no Composite output. The render pipeline early fails if neither a File Output or a Composite output node exists, so an output is guaranteed to always exist. Fixes #115983.
👍 3
Omar Emara added the
Interest
Compositing
Interest
Render Pipeline
Module
Render & Cycles
 labels 2024-01-15 12:55:14 +01:00
Omar Emara added 1 commit 2024-01-15 12:55:24 +01:00
5869b7e138 Render: Skip writing output if no Composite node exists 
This patch changes the render pipeline such that it doesn't write the
render result if the render pipeline is a compositor one whose node tree
does not have an output Composite node.

This is done to avoid double outputs when users use the File Output node
for saving the results instead. Additionally, it fixes crashes and empty
outputs when the node tree does not reference any render layers and has
no Composite output.

The render pipeline early fails if neither a File Output or a Composite
output node exists, so an output is guaranteed to always exist.

Fixes #115983 and #57699.
Brecht Van Lommel was assigned by Omar Emara 2024-01-15 12:55:42 +01:00
Sergey Sharybin was assigned by Omar Emara 2024-01-15 12:55:42 +01:00
Brecht Van Lommel was unassigned by Omar Emara 2024-01-15 12:55:58 +01:00
Sergey Sharybin was unassigned by Omar Emara 2024-01-15 12:55:58 +01:00
Omar Emara requested review from Brecht Van Lommel 2024-01-15 12:56:10 +01:00
Omar Emara requested review from Sergey Sharybin 2024-01-15 12:56:10 +01:00
Brecht Van Lommel commented 2024-01-15 13:32:53 +01:00
Owner
Copy Link

For me this raises design questions that came up in #57699.

The compositor is one step in the render pipeline. The output can go to a file, but it can also go to the image editor or feed into the sequencer. I'm not sure what the right way to think about this conceptually is.

Maybe it's, there's no Composite output, so therefore there is no render result image to pass on to the next step in the pipeline. If that next step is file output, there is nothing to write. If it's the image editor, there is nothing to display. If it's the sequencer, the strip will not generate an image but some other strip still might.

Have you checked what happens when you have a scene strip without a compositor node, but another strip that does add an image? Does it still write that, or does this change break it?

For me this raises design questions that came up in #57699. The compositor is one step in the render pipeline. The output can go to a file, but it can also go to the image editor or feed into the sequencer. I'm not sure what the right way to think about this conceptually is. Maybe it's, there's no Composite output, so therefore there is no render result image to pass on to the next step in the pipeline. If that next step is file output, there is nothing to write. If it's the image editor, there is nothing to display. If it's the sequencer, the strip will not generate an image but some other strip still might. Have you checked what happens when you have a scene strip without a compositor node, but another strip that does add an image? Does it still write that, or does this change break it?
Omar Emara commented 2024-01-15 13:50:20 +01:00
Author
Member
Copy Link

@brecht I am not sure what this means:

but it can also go to the image editor.

Because if there is no Composite node, the compositor will not write anything to the IMA_TYPE_R_RESULT viewer image. Or is there some special way that the pipeline can do that itself from the written output?

I am looking into the sequencer bit now.

@brecht I am not sure what this means: > but it can also go to the image editor. Because if there is no Composite node, the compositor will not write anything to the `IMA_TYPE_R_RESULT` viewer image. Or is there some special way that the pipeline can do that itself from the written output? I am looking into the sequencer bit now.
Omar Emara commented 2024-01-15 14:33:22 +01:00
Author
Member
Copy Link

Okay, so Scene strips suffer from the same crash issue when no composite output exist. But this is not related to this change and is orthogonal to it. I will submit a separate fix for it.

As for image viewer of render result, it will just show an empty image in that case, but it is also not related to this change, so if we want to change this behavior somehow, we can just do that in a separate branch.

Okay, so Scene strips suffer from the same crash issue when no composite output exist. But this is not related to this change and is orthogonal to it. I will submit a separate fix for it. As for image viewer of render result, it will just show an empty image in that case, but it is also not related to this change, so if we want to change this behavior somehow, we can just do that in a separate branch.
Sergey Sharybin requested changes 2024-01-15 14:33:47 +01:00
Sergey Sharybin left a comment
Owner
Copy Link

When there is no Composite output node the Composite render pass is not created, but the render result still contains all other passes, and they can be written. This change will break setup which were relying on the view layer passes (like combined) to be saved via render pipeline, and extra passes saved via File Output nodes.

Such setups might be used on the farms to have some easily viewable render output going via render pipeline, with a more complete output to a multi-layer EXR with all the other passes, possibly re-combined, denoised, cryptomatt-ed and so on.

I don't think such change is something we should be taking lightly.
If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty.

Sequencer I don't think will be a problem here, as sequencer disallows having self scene as its strip, and the flags are stored on scene-specific Render.

Even for the described current behavior the code needs to be tweaked to do proper checks for the output node.

When there is no Composite output node the Composite render pass is not created, but the render result still contains all other passes, and they can be written. This change will break setup which were relying on the view layer passes (like combined) to be saved via render pipeline, and extra passes saved via File Output nodes. Such setups might be used on the farms to have some easily viewable render output going via render pipeline, with a more complete output to a multi-layer EXR with all the other passes, possibly re-combined, denoised, cryptomatt-ed and so on. I don't think such change is something we should be taking lightly. If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty. Sequencer I don't think will be a problem here, as sequencer disallows having self scene as its strip, and the flags are stored on scene-specific `Render`. Even for the described current behavior the code needs to be tweaked to do proper checks for the output node.
source/blender/render/intern/pipeline.cc Outdated
@ -1157,2 +1157,4 @@
}
/** Returns true if the scene has a composite output. */
static bool compositor_has_output(Scene *scene)
Sergey Sharybin commented 2024-01-15 14:09:32 +01:00
Owner
Copy Link

The naming is quite close to the existing node_tree_has_compositor_output, think its better to:

  • Rename node_tree_has_compositor_output to node_tree_has_any_compositor_output
  • Rename compositor_has_output to node_tree_has_composite_output
The naming is quite close to the existing `node_tree_has_compositor_output`, think its better to: - Rename `node_tree_has_compositor_output` to `node_tree_has_any_compositor_output` - Rename `compositor_has_output ` to `node_tree_has_composite_output`
OmarEmaraDev marked this conversation as resolved
source/blender/render/intern/pipeline.cc Outdated
@ -1159,0 +1174,4 @@
for (const bNode *node : node_tree->all_nodes()) {
if (node->type == CMP_NODE_COMPOSITE && node->flag & NODE_DO_OUTPUT &&
!(node->flag & NODE_MUTED))
Sergey Sharybin commented 2024-01-15 14:10:23 +01:00
Owner
Copy Link

I don't think the composite node can be muted.

There also needs to be recursion into node groups, as the composite output might be in a node group.

I don't think the composite node can be muted. There also needs to be recursion into node groups, as the composite output might be in a node group.
Omar Emara commented 2024-01-15 14:56:55 +01:00
Author
Member
Copy Link

They can be muted through RNA, just not through the mute operator. For instance, see report #109676.

Also, I just realized that this is a difference between CPU and GPU compositor, because the realtime compositor only considers output nodes in the root context, and I think this is the correct behavior. We do not have a mechanism for tagging active composite outputs across node groups, and allowing File Outputs inside node groups is rather dangerous considering node groups are shared.

Viewer nodes are an exception to this rule, in that we consider the active context, and not the root context, since it is useful for interactive compositing and is safe to do.

They can be muted through RNA, just not through the mute operator. For instance, see report #109676. Also, I just realized that this is a difference between CPU and GPU compositor, because the realtime compositor only considers output nodes in the root context, and I think this is the correct behavior. We do not have a mechanism for tagging active composite outputs across node groups, and allowing File Outputs inside node groups is rather dangerous considering node groups are shared. Viewer nodes are an exception to this rule, in that we consider the active context, and not the root context, since it is useful for interactive compositing and is safe to do.
Brecht Van Lommel commented 2024-01-15 14:37:40 +01:00
Owner
Copy Link

Heh, I'm not sure what you mean with that question.

I just meant to say that the next step after compositing can be file output, image display or feeding into the sequencer. Not that there is anything particular to be solved regarding the image editor, if the compositor does not write to the IMA_TYPE_R_RESULT viewer image when there is no composite node that is fine.

Heh, I'm not sure what you mean with that question. I just meant to say that the next step after compositing can be file output, image display or feeding into the sequencer. Not that there is anything particular to be solved regarding the image editor, if the compositor does not write to the `IMA_TYPE_R_RESULT` viewer image when there is no composite node that is fine.
Omar Emara commented 2024-01-15 15:12:23 +01:00
Author
Member
Copy Link

If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty.

I suppose I can do that if there is an agreement.

> If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty. I suppose I can do that if there is an agreement.
Omar Emara added 2 commits 2024-01-15 15:38:39 +01:00
Brecht Van Lommel commented 2024-01-15 15:41:21 +01:00
Owner
Copy Link

If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty.

I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write.

That seems like a sensible thing to do regardless, and if it solves the crash then great.

That does not resolve #57699, which would be nice if we could get solve additionally, in a separate PR if needed. I think it may be reasonable for non-multilayer file formats to skip writing files when there is no composite node. With the idea that in such a case the "composite" layer in the render result is effectively empty or missing, so there is nothing to write. With versioning we could insert a composite node if we wanted to avoid breaking compatibility, at least as far as .blend files are concerned, not scripts.

My only concern is if that logic would be too difficult to understand for users. But I think in practice this only happens when you already created a file output node, because if you have no compositing output nodes Blender will refuse to even start rendering. So it may be pretty natural.

> If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty. I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write. That seems like a sensible thing to do regardless, and if it solves the crash then great. That does not resolve #57699, which would be nice if we could get solve additionally, in a separate PR if needed. I think it may be reasonable for non-multilayer file formats to skip writing files when there is no composite node. With the idea that in such a case the "composite" layer in the render result is effectively empty or missing, so there is nothing to write. With versioning we could insert a composite node if we wanted to avoid breaking compatibility, at least as far as .blend files are concerned, not scripts. My only concern is if that logic would be too difficult to understand for users. But I think in practice this only happens when you already created a file output node, because if you have no compositing output nodes Blender will refuse to even start rendering. So it may be pretty natural.
Sergey Sharybin commented 2024-01-15 16:09:02 +01:00
Owner
Copy Link

I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write.

On a functional level indeed that is a more clear saying. I was more looking from technical implementation, as it is not that easy to tell if something got written to the pass or not, and there are comments like

/* A render-layer should always have a "Combined" pass. */
 render_layer_add_pass(rr, rl, 4, "Combined", view, "RGBA", false);

Perhaps we can remove all all non-allocated passes after render engine is done rendering, and check if render result has any passes before writing. Not sure if it breaks assumptions somewhere else about the "render-layer should always have combined pass" . Needs a deeper check.

> I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write. On a functional level indeed that is a more clear saying. I was more looking from technical implementation, as it is not that easy to tell if something got written to the pass or not, and there are comments like ``` /* A render-layer should always have a "Combined" pass. */ render_layer_add_pass(rr, rl, 4, "Combined", view, "RGBA", false); ``` Perhaps we can remove all all non-allocated passes after render engine is done rendering, and check if render result has any passes before writing. Not sure if it breaks assumptions somewhere else about the "render-layer should always have combined pass" . Needs a deeper check.
Omar Emara commented 2024-01-22 13:21:41 +01:00
Author
Member
Copy Link

The suggested behavior is currently implemented, should I do something else?

The suggested behavior is currently implemented, should I do something else?
Brecht Van Lommel commented 2024-01-22 16:26:02 +01:00
Owner
Copy Link

I think the suggestion from @Sergey was to still save the result from any render layer nodes, which the implementation in this PR doesn't do?

I think the suggestion from @Sergey was to still save the result from any render layer nodes, which the implementation in this PR doesn't do?
Omar Emara commented 2024-01-22 16:30:18 +01:00
Author
Member
Copy Link

@brecht I moved the check in the else block of compositor_needs_render(), so it will only skip writing if no render layer node exists. But maybe I missed something, I will retest after I finish the thing at hand and confirm that.

@brecht I moved the check in the else block of `compositor_needs_render()`, so it will only skip writing if no render layer node exists. But maybe I missed something, I will retest after I finish the thing at hand and confirm that.
Brecht Van Lommel approved these changes 2024-01-22 16:33:19 +01:00
Brecht Van Lommel left a comment
Owner
Copy Link

Ah, I missed that. Looks good to me then.

Ah, I missed that. Looks good to me then.
Sergey Sharybin commented 2024-01-22 17:02:53 +01:00
Owner
Copy Link

This patch still breaks behavior when the compositor output is in a node group. This isn't something that should be done as a bi-product of another fix, and only mentioned in an inlined comment.

If the render pipeline is to only consider output from the "root" tree it needs to be done consistently, and breaking changes are to be mentioned explicitly.

On a code side it is confusing why node_tree_has_any_compositor_output checks for outputs in the node groups, but node_tree_has_compositor_output does not.

This patch still breaks behavior when the compositor output is in a node group. This isn't something that should be done as a bi-product of another fix, and only mentioned in an inlined comment. If the render pipeline is to only consider output from the "root" tree it needs to be done consistently, and breaking changes are to be mentioned explicitly. On a code side it is confusing why `node_tree_has_any_compositor_output` checks for outputs in the node groups, but `node_tree_has_compositor_output ` does not.
Omar Emara added 2 commits 2024-01-23 14:11:46 +01:00
Omar Emara requested review from Sergey Sharybin 2024-01-23 14:13:14 +01:00
Sergey Sharybin approved these changes 2024-01-24 10:05:18 +01:00
Omar Emara merged commit c60321f6dc into main 2024-01-24 12:00:59 +01:00
Omar Emara deleted branch skip-no-composite-write 2024-01-24 12:01:05 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Brecht Van Lommel
Sergey Sharybin
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117129
No description provided.
Delete Branch "OmarEmaraDev/blender:skip-no-composite-write"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?