Fullframe compositor: unify behavior of size inference with realtime-compositor #114755

Habib Gahbiche · 2023-11-12T11:34:00+01:00

Habib Gahbiche commented

2023-11-12 11:34:00 +01:00

Make size inference consistent with the viewport compositor (from a user's perspective). This patch uses constat folding to create a constant output out of constant inputs. This is consistent with the results of the realtime compositor.

Example with translate node:

main	this patch

Notice how previously the translate node infers its cavas size from the viewer node first, and then applies translation. Now, since the input is constant there is no translation to be made, and the final render result is therefore not translated. This applies to all other nodes included in this patch.

Following nodes are not included in this patch because they need further refactoring or discussion:

Input nodes
Defocus: can't be constant because it implicitly relies on an input operation
Flip node
Inpaint
Kuwahara classic
Lens distortion
Split viewer node (already addressed in #114245)
Sun beams (seems to need changes on GPU side)
Tone map
Vector Blur

Make size inference consistent with the viewport compositor (from a user's perspective). This patch uses constat folding to create a constant output out of constant inputs. This is consistent with the results of the realtime compositor. Example with translate node: | main | this patch | | ----------- | ----------- | | ![before](/attachments/06dca081-28c9-45ca-89b4-a43a48149682) | ![after](/attachments/98e102c1-bbf5-4eff-b994-e37dcf56075e) | Notice how previously the translate node infers its cavas size from the viewer node first, and then applies translation. Now, since the input is constant there is no translation to be made, and the final render result is therefore not translated. This applies to all other nodes included in this patch. Following nodes are not included in this patch because they need further refactoring or discussion: - Input nodes - Defocus: can't be constant because it implicitly relies on an input operation - Flip node - Inpaint - Kuwahara classic - Lens distortion - Split viewer node (already addressed in #114245) - Sun beams (seems to need changes on GPU side) - Tone map - Vector Blur

before.jpg

146 KiB

after.jpg

126 KiB

Habib Gahbiche added 2 commits 2023-11-12 11:34:11 +01:00

6f0a23eb63 Fullframe compositor: unify behavior of size inference with realtime-compositor

buildbot/vexp-code-patch-coordinator Build done. Details

7f061ab119 Merge remote-tracking branch 'origin/main' into com-unify-size-inference

Habib Gahbiche commented

2023-11-12 11:42:29 +01:00

@blender-bot build

Habib Gahbiche requested review from Omar Emara 2023-11-12 17:10:01 +01:00

Omar Emara commented

2023-11-13 12:00:53 +01:00

Such constant folding only happens when the values are known at compile-time, correct?
So if a single value is computed at runtime, from a reduction operation like the Levels node, the behavior will revert to its original method, right?

Such constant folding only happens when the values are known at compile-time, correct? So if a single value is computed at runtime, from a reduction operation like the Levels node, the behavior will revert to its original method, right?

Habib Gahbiche commented

2023-11-13 20:15:21 +01:00

Constant folding of the node graph happens every time COM_execute() gets called. It replaces operations such as TranslateOperation with ConstantOperation at runtime, right after building the graph when all inputs are constant. Here constant means the input (and therefore output) does not vary across xy-coordinates.
The interesting function is ConstantOperation *ConstantFolder::fold_operation(NodeOperation *operation) in COM_ConstantFolder.cc

All unit tests are passing for this patch so the current behavior, where an image input is connected to the node, remains the same.

Constant folding of the node graph happens every time `COM_execute()` gets called. It replaces operations such as `TranslateOperation` with `ConstantOperation` at runtime, right after building the graph when all inputs are constant. Here constant means the input (and therefore output) does not vary across xy-coordinates. The interesting function is `ConstantOperation *ConstantFolder::fold_operation(NodeOperation *operation)` in `COM_ConstantFolder.cc` All unit tests are passing for this patch so the current behavior, where an image input is connected to the node, remains the same.

Omar Emara commented

2023-11-13 21:47:59 +01:00

By compile time, I meant the node tree conversion to operations. Does it work as expected when using the Levels node as I mentioned?

Habib Gahbiche commented

2023-11-13 22:28:13 +01:00

Not sure if this answers your question, but levels is not included in this patch, so when connected to an image it will behave as before, but when it's input is not connected to an input it will behave like the realtime compositor when connected to a translate node:

connected:

not connected:

Not sure if this answers your question, but levels is not included in this patch, so when connected to an image it will behave as before, but when it's input is not connected to an input it will behave like the realtime compositor when connected to a translate node: connected: <img src="/attachments/3cdc48bd-3e4d-4efe-9333-417be647c0c8" width="400" /> not connected: <img src="/attachments/db6614ae-be4a-46f9-8b48-9a17fe53aa24" width="400" />

Omar Emara commented

2023-11-14 18:48:18 +01:00

I am talking about this setup. The Levels node outputs a single value, yet, it is treated as an image by the translate node. So my point is that constant folding can't really be used to achieve that behavior in all cases. Or maybe I misunderstand your intention, what do you think?

I am talking about this setup. The Levels node outputs a single value, yet, it is treated as an image by the translate node. So my point is that constant folding can't really be used to achieve that behavior in all cases. Or maybe I misunderstand your intention, what do you think? ![20231114-193801.png](/attachments/7b2be333-7fa8-4e19-b28f-c82161561dd5)

20231114-193801.png

523 KiB

Habib Gahbiche commented

2023-11-14 23:06:03 +01:00

That's the setup I tried to demonstrate in my previous post. For some reason images are not showing properly...

I don't think we can cover that case using the way constant folding is implemented now, but I think it should be possible to achieve the result we want with a relatively small change, e.g. by adding a dummy constant operation between CalculateMeanOperation and output of Levels node, or by supporting non-constant input and constant output in ConstantFolder. I haven't looked much into that yet.

This patch includes only the straightforward nodes where only one line of code is needed. I plan to unify the behavior of the rest in separate patches, though I am not sure constant folding is the way to go for all the other nodes

That's the setup I tried to demonstrate in my previous post. For some reason images are not showing properly... I don't think we can cover that case using the way constant folding is implemented now, but I think it should be possible to achieve the result we want with a relatively small change, e.g. by adding a dummy constant operation between `CalculateMeanOperation` and output of Levels node, or by supporting non-constant input and constant output in `ConstantFolder`. I haven't looked much into that yet. This patch includes only the straightforward nodes where only one line of code is needed. I plan to unify the behavior of the rest in separate patches, though I am not sure constant folding is the way to go for all the other nodes

Omar Emara commented

2023-11-15 09:22:19 +01:00

I am generally skeptical about the existence of constant folding for the compositor, but I can accept it for now. Just one question, does it make sense to set can_be_constant to true by default and instead exempt nodes from constant folding?

I am generally skeptical about the existence of constant folding for the compositor, but I can accept it for now. Just one question, does it make sense to set `can_be_constant` to true by default and instead exempt nodes from constant folding?

Habib Gahbiche commented

2023-11-16 14:17:53 +01:00

does it make sense to set can_be_constant to true by default?

That could work too, I was hoping this way shows the behavior more explicitly in operations. I don't have a strong opinion though, either way is fine for me.

> does it make sense to set `can_be_constant` to true by default? That could work too, I was hoping this way shows the behavior more explicitly in operations. I don't have a strong opinion though, either way is fine for me.

Omar Emara commented

2023-11-16 14:19:52 +01:00

I think we should go with that route, since our target is that all nodes could produce constant values.

Habib Gahbiche commented

2023-11-16 14:34:54 +01:00

Ideally we shouldn't need that flag at all, since all operations can be constant, and the flag is_constant_operation exists for operations that are definately constant.

I will refactor in a later patch.

Ideally we shouldn't need that flag at all, since all operations *can* be constant, and the flag `is_constant_operation` exists for operations that are definately constant. I will refactor in a later patch.

Omar Emara approved these changes 2023-11-21 17:34:39 +01:00

Habib Gahbiche added 2 commits 2023-11-26 12:08:44 +01:00

45ee5f76cc Merge remote-tracking branch 'origin/main' into com-unify-size-inference

9f1d208c3c Merge remote-tracking branch 'origin/main' into com-unify-size-inference

Habib Gahbiche merged commit 9cc038f580 into main

2023-11-26 12:14:47 +01:00

Habib Gahbiche referenced this issue from a commit

2023-11-26 12:14:48 +01:00

Fullframe compositor: unify behavior of size inference with realtime-compositor

Habib Gahbiche deleted branch com-unify-size-inference

2023-11-26 12:14:49 +01:00

Hans Goudey referenced this issue from a commit