Compositor: add new node: Kuwahara filter #107015
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#107015
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "zazizizou/blender:com-kuwahara-filter-node"
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?
Summary:
The filter is used to reduce noise while preserving edges. It can be used to create a cartoon effect from photorealistic images.
The idea is to offer two modes:
This patch implements both approaches above as multi-threaded operations for the full-frame compositor. The attached gif demonstrates how to use the filter.
WIP Progress:
Implement classic kuwahara filter
Implement anisotropic kuwahara filter for 3 channel RGB images
Implement orientation from structure tensor as a separate operation. (Won't do)Performance improvements
Regression testing (will be submitted after merging)
Validate UI with users: https://devtalk.blender.org/t/compositor-new-node-kuwahara-filter/29205
Limitations:
WIP: ompositor: add new node: Kuwahara filterto WIP: Compositor: add new node: Kuwahara filterJust a quick pass over the code.
Did not have chance to apply the patch and play with it yet, but the implementation overall seems fine for the single channel approach.
@ -0,0 +1,24 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2011 Blender Foundation. */
Use the current year for the new files added. This also applies to other new files in this PR.
@ -0,0 +9,4 @@
{
this->add_input_socket(DataType::Color);
this->add_output_socket(DataType::Color);
this->set_kernel_size(4.4f);
This is a bit confusing. The kernel size is an integer in the API, and here we pass a floating point of
4.4f
. Should this be just 4?Right, the compiler shows a warning indeed. I was trying to understand where defaults get set. Will fix later
@ -0,0 +32,4 @@
float input_value[4];
image_reader_->read_sampled(input_value, x, y, sampler);
output[0] = input_value[0] + 1.0;
Is this some stub implementation for tiled system, or is it some sort of step of the actual algorithm?
If it's the former, then it's all fine for now. If that's the latter I'd like to understand how it works :)
This is a stub implementation. I was just making sure I understood the channel ordering correctly. Will remove to avoid confusion.
@ -0,0 +80,4 @@
int xx = x + dx;
int yy = y + dy;
if (xx >= 0 && yy >= 0 && xx < area.xmax && yy < area.ymax) {
Suggest using early
continue
and un-indent the block.Thanks for looking into the patch @Sergey! As you noticed this is still work in progress, so it was not really ready for code review. I will upload my progress with the anisotropic filter soon. I am planning small changes to file structure and will address your comments in a separate patchset
@blender-bot package windows
Package build started. Download here when ready.
@blender-bot package windows
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
On the code side it seems good, just some cosmetic feedback.
I did not see anything immediately problematic in the code, but I am running into some artifacts when using the anisotropic node. used Einar shading test file from the Studio website. Similar blackening can be seen with other images, i.e. Lenna. Not sure if it is expected. Seems a bit odd.
The performance we might need to look into, but I would not consider it a requirement for the merge.
What we need to figure out is to how to hide the node when full-frame compositor is not used. I did not check on that yet.
Ans also maybe add a warning in the node when Tiled compositor is used. So that if someone opens file saved with full-frame has a better time figuring out why things look different?
@ -0,0 +1,22 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2011 Blender Foundation. */
2023
@ -69,1 +70,4 @@
void FastGaussianBlurOperation::set_size(int size_x, int size_y)
{
// todo: there should be a better way to use the operation without knowing specifics of the blur
We use C style comments, even in C++:
/* ... */
@ -0,0 +19,4 @@
this->add_output_socket(DataType::Color);
// output for debug
// todo: remove
Is it something you planned to do before the actual merge?
@ -0,0 +44,4 @@
float y,
PixelSampler sampler)
{
/* Not implemented */
Should we consider something from:
So that if someone saves file with the Kuwahara node in it and someone opens it without enabling full-frame compositor we do not leave the output uninitialized?
@ -0,0 +33,4 @@
float y,
PixelSampler sampler)
{
/* Not implemented */
Same as above.
@ -0,0 +6,4 @@
*/
#include "COM_node_operation.hh"
//#include "node_composite_util.hh"
Remove the commented out code.
@ -9245,0 +9256,4 @@
prop = RNA_def_property(srna, "size", PROP_INT, PROP_NONE);
RNA_def_property_int_sdna(prop, NULL, "size");
RNA_def_property_ui_range(prop, 4, 50, 1, -1);
This doesn't seem to work. User can still set values outside the specified range. This is also the case for Vector Blur. Is there another way to specify UI range?
The UI range is the soft limit. It applies when an artist drags the slider, but it is still possible to click on the property and type a value which is within a hard limit.
The hard limit is controlled by
RNA_def_property_range
.You can see example in the Subsurf modifier:
This did solve it, thanks!
@Sergey it was easier to have a naive implementation for tiled compositor than figuring out how to hide the node without causing problems with versioning, so I did that : ).
There are two reasons for this:
double
instead offloat
seems to solve it.I also solved the issue with black pixels at the border by clamping, as suggested during our meeting on Tuesday.
@blender-bot package
Package build started. Download here when ready.
WIP: Compositor: add new node: Kuwahara filterto Compositor: add new node: Kuwahara filterThanks for the update! I'll give it a closer look in a bit. Some quick replies for now.
Lovely! It also makes it easier to merge early on :)
On the CPU it is probably fine, but for the GPU it might become a problem.
But to me it seems fine to move on with doubles for now.
Isn't the number of regions dependent on a frame? So not sure how we can solve it by a static limit.
Also, don't think I've touched any node settings. So maybe the default needs to be changed?
I gave this a quick test, super nice, thanks a lot for implementing it!
Two observations:
Finally got time to test the new build and look into the code.
I think we are pretty much there!
There are few small inlined comments, which can probably be applied in other places as well.
The bigger thing i am not sure about is the duplication of code. Unfortunately, I do not have great solution to avoid it. On the one hand maybe we can template the logic and pass pixel accessor as a template argument, but it will have other issues.
I don't think it worth obfuscating the full-frame compositor code. A bit of a hard call, but maybe it is better to accept some duplication which is locally more readable, and then remove the duplication together with the tiled compositor :)
What do you think?
@ -0,0 +102,4 @@
if (xx < 0) {
xx = 0;
}
if (xx >= this->get_width()) {
It might help making it an explicit
const int height = this->get_height()
outside of the loop. I am not sure compiler is smart enough to do it for us.Although, also not suer it will give measurable time impact, but still feels like a good thing to do.
@ -0,0 +115,4 @@
double ddx2 = (double)dx2;
double ddy2 = (double)dy2;
double theta = atan2(ddy2, ddx2) + M_PI;
int t = static_cast<int>(floor(theta / angle)) % n_div_;
For the arithmetic types you can do
int(foo)
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_CastProbably will make code a bit easier to read.
@ -0,0 +135,4 @@
double de = 0.0;
double nu = 0.0;
for (int i = 0; i < n_div_; i++) {
mean[i] = weight[i] != 0 ? mean[i] / weight[i] : 0.0;
In other performance critical areas we do
Again, not something which i know for sure will show performance impact, but might worth doing so nevertheless.
@Sergey @SimonThommes Thanks for looking into the patch!
The kernel itself is split into
![](https://i.ibb.co/rFfhngN/classic-vs-anisotropic.jpg)
n_div
regions (see image below from the original paper), that's what I meant by regions. So if we always setn_div = 8
butkernel_size < 4
, then some regions might not get covered and get set to 0. I could reproduce this problem only with anisotropic variation.This is the case for almost all other operations. In my opinion, I don't think it's worth doing it for all operations or even just for the new filter, when we are planning to remove the tiled implementation anyways.
You're right, I removed the clamping. The filter still won't produce values larger than those in the image itself, but now it shouldn't clamp unclamped input images.
I'm hearing this more often than I expected... I see improvement potential in both variations actually (see todos in code), I still want to prioritize other tasks, e.g. merging full-frame compositor, but if the speed makes the filter unusable than I might have to look into it sooner. I will bring up priorities in the next VFX meeting on Tuesday
There are some code style and minor performance remarks.
@zazizizou , For the efficiency of moving forward do you mind if i address them in your branch, so that we can merge early on?
I was also experimenting with replacing the per-channel look using
float3
anddouble3
. The naive conversion gave 2x speedup. The unfortunate thing is that I will need to first commit some improvements to the BLI_math to implement some missing functionality. And in any case, it would be nice to have the naive implementation first, so that we can create regression test and verify the "vectorized" implementation. I can take care of that once the patch is merged.@ -0,0 +24,4 @@
converter.add_operation(operation);
converter.map_input_socket(get_input_socket(0), operation->get_input_socket(0));
converter.map_output_socket(get_output_socket(0), operation->get_output_socket());
} break;
break
should be inside of the block: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Operators_and_Statements@ -0,0 +231,4 @@
for (int ch = 0; ch < 3; ch++) {
Vector<double> mean(n_div_, 0.0f);
I've pretty much skipped the tiled implementation, but same note can be applied there.
I think we should move allocations to as high level as possible.
@ -0,0 +251,4 @@
int xx = x + dx2;
int yy = y + dy2;
/* Clamp image to avoid artefacts at borders. */
Think this can be shortened to:
(add
#include BLI_math_base.h
at the top of the file).@ -0,0 +3,4 @@
#include "COM_KuwaharaClassicOperation.h"
extern "C" {
No need to `extern "C"`` here, the header does it already.
Generally, we should not be adding such statements around
#include
statements, as it makes it very easy to break things when things move to C++. Some headers might need the linking specializer, and for those it is to be changed in the header itself.@Sergey thanks for the tips. I updated the patch with the minor changes. Sure, feel free to push further changes! I am planning to look into the performance this weekend.
I did some final round of cleanup.
As mentioned before, there there are some unideal aspects, but I would like to solve them incrementally from the main branch. This will include:
I already have some local code for the performance, but I'd really like to first have a more simple/naive and tested implementation in-place to generate regression files.
Thank you @Sergey ! I prepared some files for regression testing already, I will commit them to svn after submitting this patch