Compositor: Rewrite and optimize Double Edge Mask node #117545

Merged

Omar Emara merged 4 commits from OmarEmaraDev/blender:rewrite-double-edge-mask-node into main

2024-01-30 19:49:47 +01:00

Omar Emara commented

2024-01-26 14:43:47 +01:00

Member

This patch rewrites and optimizes the Double Edge Mask node to be orders
of magnitude faster. For a 1k complex mask, it is 650x faster, while for
a 1k simple mask, it is only 50x faster.

This improvement is attributed to the use of a new Jump Flooding
algorithm as well as multi-threading, matching the GPU implementation.

The result of the new implementation differs in that the boundaries of
the masks are now identified as the last pixels inside the mask.

This patch rewrites and optimizes the Double Edge Mask node to be orders of magnitude faster. For a 1k complex mask, it is 650x faster, while for a 1k simple mask, it is only 50x faster. This improvement is attributed to the use of a new Jump Flooding algorithm as well as multi-threading, matching the GPU implementation. The result of the new implementation differs in that the boundaries of the masks are now identified as the last pixels inside the mask.

Omar Emara added the

labels 2024-01-26 14:43:48 +01:00

Omar Emara added 1 commit 2024-01-26 14:43:58 +01:00

5da37b43e6 Compositor: Rewrite and optimize Double Edge Mask node

This patch rewrites and optimizes the Double Edge Mask node to be orders
of magnitude faster. For a 1k complex mask, it is 650x faster, while for
a 1k simple mask, it is only 50x faster.

This improvement is attributed to the use of a new Jump Flooding
algorithm as well as multi-threading, matching the GPU implementation.

The result of the new implementation differs if the Inner Edge option is
set to Adjacent Only and the Inner Mask is not inside the Outer Mask.
The difference is due the condition that identifies the edges of the
Inner Mask.

Omar Emara requested review from Sergey Sharybin 2024-01-26 14:44:12 +01:00

Omar Emara commented

2024-01-26 14:47:18 +01:00

Author

Member

@Sergey I still need to figure out and give more details on the differences, the old code is a bit convoluted, just a tiny bit ☺️. But the base code should be ready for review.

@Sergey I still need to figure out and give more details on the differences, the old code is a bit convoluted, just a tiny bit :relaxed:. But the base code should be ready for review.

Hans Goudey reviewed 2024-01-26 15:27:10 +01:00

Hans Goudey left a comment

Member

Just reading this out of curiosity (nice speedup!) and noticed one thing. Array references should typically become MutableSpan instead. That clarifies that functions don't care about ownership, and makes them container-agnostic.

Just reading this out of curiosity (nice speedup!) and noticed one thing. Array references should typically become `MutableSpan` instead. That clarifies that functions don't care about ownership, and makes them container-agnostic.

source/blender/compositor/operations/COM_DoubleEdgeMaskOperation.cc Outdated

						
				@ -5,2 +5,4 @@

				#include <cstdlib>

				#include "BLI_math_vector.hh"

				#include "BLI_math_vector_types.hh"

Hans Goudey commented

2024-01-26 15:25:29 +01:00

Member

BLI_math_vector.hh includes BLI_math_vector_types.hh out of necessity, no need to write them both here

`BLI_math_vector.hh` includes `BLI_math_vector_types.hh` out of necessity, no need to write them both here

OmarEmaraDev marked this conversation as resolved

source/blender/compositor/operations/COM_DoubleEdgeMaskOperation.cc Outdated

						
				@ -1283,2 +130,2 @@

				    MEM_freeN(gbuf);

				  }

				  const int2 size = int2(this->get_width(), this->get_height());

				  Array<int2> inner_boundary = Array<int2>(size_t(size.x) * size.y);

Hans Goudey commented

2024-01-26 15:26:41 +01:00

Member

Can be written more simply as

Array<int2> inner_boundary(size_t(size.x) * size.y);

Can be written more simply as ``` Array<int2> inner_boundary(size_t(size.x) * size.y);

OmarEmaraDev marked this conversation as resolved

Omar Emara added 1 commit 2024-01-26 16:07:10 +01:00

6f838711e5 Address review

Omar Emara commented

2024-01-26 16:14:44 +01:00

Author

Member

Thanks @HooglyBoogly! Do you happened to know if the jump_flooding function will have "Return value optimization", or will it copy the array on return? I am worried that the pointer and deference operations will confuse the compiler somehow.

Thanks @HooglyBoogly! Do you happened to know if the [`jump_flooding`](https://projects.blender.org/blender/blender/src/commit/6f838711e5a4c5375745dacb262795aa07a0341f/source/blender/compositor/algorithms/COM_JumpFloodingAlgorithm.cc#L103) function will have "Return value optimization", or will it copy the array on return? I am worried that the pointer and deference operations will confuse the compiler somehow.

Omar Emara commented

2024-01-26 19:26:29 +01:00

Author

Member

The test failures is due to two things:

The old implementation identifies the edges as the pixels right outside of the masks, while the new implementation identifies the edges as the pixels right at the edge of the masks. I would argue that the latter is more expected, because we tend to expect gradients to include the zero value.
The old implementation only considers outside edges that are not inside the inner mask, while the new implementation considers all outside edges. This is equivalent to the Inner Edge option, but for outer edges. So we could consider adding it as an option, or choosing either one.

Another difference that is not related to test failure is:

The old implementation outputs nothing when no inner mask exist in the outer mask in the Adjacent Only mode.

More generally, by looking at the history of the node, there is no adjacency information, so the UI and documentation are probably wrong. And it seems the node was added as a workaround for feathered masks. See https://archive.blender.org/wiki/index.php/User:Xgl_asyliax/ and https://blenderartists.org/t/yay-double-edge-matte-might-get-in-to-trunk/523603/15.

The test failures is due to two things: - The old implementation identifies the edges as the pixels right outside of the masks, while the new implementation identifies the edges as the pixels right at the edge of the masks. I would argue that the latter is more expected, because we tend to expect gradients to include the zero value. - The old implementation only considers outside edges that are not inside the inner mask, while the new implementation considers all outside edges. This is equivalent to the Inner Edge option, but for outer edges. So we could consider adding it as an option, or choosing either one. Another difference that is not related to test failure is: - The old implementation outputs nothing when no inner mask exist in the outer mask in the Adjacent Only mode. More generally, by looking at the history of the node, there is no adjacency information, so the UI and documentation are probably wrong. And it seems the node was added as a workaround for feathered masks. See https://archive.blender.org/wiki/index.php/User:Xgl_asyliax/ and https://blenderartists.org/t/yay-double-edge-matte-might-get-in-to-trunk/523603/15.

Sergey Sharybin commented

2024-01-30 10:24:06 +01:00

Owner

The simple use-case in the double_edge_mask.blend seems to work very nicely with this patch.

I was doing some digging of the failing tests, trying to identify the cause, or the practicality of it. The inner and outer masks are barely touching there, so i've expanded the inner mast a bit in the double_edge_mask_tricky.blend. Intuitively one would probably expect smooth feather, with soft transition from white in the center to the black at the outer edges of the feather. However, the new implementation doesn't have such smooth edges. Is it something that is covered by the difference in the second point of the tests faulures?

The simple use-case in the `double_edge_mask.blend` seems to work very nicely with this patch. I was doing some digging of the failing tests, trying to identify the cause, or the practicality of it. The inner and outer masks are barely touching there, so i've expanded the inner mast a bit in the `double_edge_mask_tricky.blend`. Intuitively one would probably expect smooth feather, with soft transition from white in the center to the black at the outer edges of the feather. However, the new implementation doesn't have such smooth edges. Is it something that is covered by the difference in the second point of the tests faulures?

double_edge_mask.blend

940 KiB

double_edge_mask_tricky.blend

428 KiB

Sergey Sharybin added this to the Compositing project 2024-01-30 10:28:42 +01:00

Omar Emara added 2 commits 2024-01-30 15:12:46 +01:00

2261ecb90d Merge branch 'main' into rewrite-double-edge-mask-node

ba2c9a9e27 Match outer mask boundary between CPU and GPU

Sergey Sharybin approved these changes 2024-01-30 18:25:36 +01:00

Sergey Sharybin left a comment

Owner

The code seems fine, and the behavioir seems to be more intuitive.

Perhaps we should add some Dilate node in the regression tests to make inner and outer masks to actually intersect.

The code seems fine, and the behavioir seems to be more intuitive. Perhaps we should add some Dilate node in the regression tests to make inner and outer masks to actually intersect.

Omar Emara referenced this issue from a commit

2024-01-30 19:49:46 +01:00

Compositor: Rewrite and optimize Double Edge Mask node

Omar Emara merged commit 049b0e6539 into main

2024-01-30 19:49:47 +01:00

Omar Emara deleted branch rewrite-double-edge-mask-node

2024-01-30 19:49:52 +01:00

Jonas Dichelle referenced this issue from a commit

2024-02-08 17:29:06 +01:00

Compositor: Rewrite and optimize Double Edge Mask node

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

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#117545

No description provided.