Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Group Reviewers

Cycles

Summary

This patch introduces Cryptomatte output passes to Cycles.

There are two modes:
"Accurate" mode, which is CPU only, tracks all
IDs encountered in a pixel before it sorts them by coverage and strips
any that exceed the number of selected Cryptomatte layers. Since that
relies on dynamic memory allocations, it is CPU only.
The other mode keeps track of no more IDs than the requested number of
layers. When the number of IDs for a given pixel exceed the number of
layers, it is possible that an ID with a higher coverage gets rejected
in favor of an ID with lower coverage, if the latter was sampled first.
It would be good improvement in the future to allow GPU renders too to
track any number of IDs per pixel, regardless of the number of
Cryptomatte layers.

In order to support a user-selectable number of Cryptomatte layers,
Cycles passses are now not only distinguished by type but also by name.
That way, any number of passes of type PASS_CRYPTOMATTE can be added
to a render. Going forward, this could become useful for other types too,
should we introduce light path expressions or custom AOVs.

Diff Detail

Branch

cycles_cryptomatte

Build Status

Buildable 1846
Build 1846: arc lint + arc unit

There are a very large number of changes, so older changes are hidden. Show Older Changes

intern/cycles/kernel/kernel_passes.h
292	This doesn't work reliably for CUDA and OpenCL, threads can be rendering multiple samples in the same pixels simultaneously. How to implement `kernel_write_id_slots` with atomic ops I'm not sure of though, seems rather tricky.
intern/cycles/kernel/kernel_path_branched.h
561	Why is this matte weight needed? It seems to be throughput but with transparency averaged per surface, it's not clear to me why that is better than averaging the throughput when writing the pass, any specific cases where that failed?
intern/cycles/util/util_murmurhash.h
76	Functions should have lower case names, also don't think we need the `x86_32` postfix if it's going to be used for all CPU architectures. I think the implementation can also be moved into a `util_murmurhash.cpp`, not need to have it in the header.

This revision now requires changes to proceed.Jul 19 2018, 1:21 AM

intern/cycles/kernel/kernel_passes.h
292	That's a good one. I wrote this code initially against an older code base and tested only the CUDA side, where this was never an issue. I would think that there should be a way of doing this using atomic compare and swaps.

intern/cycles/blender/blender_sync.cpp
522	`string_startswith()` ?
555–557	Indentation.
626	`std::min` -> this is what `util_algorithm.h` is for.
intern/cycles/blender/blender_sync.h
70–71	Indentation.
intern/cycles/device/device_cpu.cpp
56	Include statements are supposed to be sorted alphabetically. See `render/buffers.h` above.
intern/cycles/kernel/kernel_passes.h
297–334	I'm sure this madness of ifdef is easily avoidable.
intern/cycles/render/buffers.cpp
239	First you go away from `string` to `char` when calling this function, then you go away from `string` to `char` here in the pass? Whats' the point? Just compare string to string.
intern/cycles/util/util_murmurhash.h
127–128	`util_algorithm.h`

Cycles: Cryptomatte writing now uses atomics on GPUs to avoid race conditions.
Cycles: Code style, fixed typos moving murmurhash to its own cpp file.

Harbormaster completed building B1846: Diff 11441.Jul 20 2018, 12:31 PM

I think I found a way to write id passes using atomics.

Cycles: Compile fixes for OpenCL kernel.

intern/cycles/kernel/kernel_globals.h
73–75	I benchmarked the Victor scene at 50% size, all Cryptomatte options enabled. Compiled with Visual Studio 2015 running on 2x Xeon E5-2660v2, that is 40 threads. Accurate mode: 8m21s Stochastic mode: 8m18s Cryptomatte off: 8m10s So STL appears to be a measurable overhead, but it is below 1% of total render time for this scene. A custom linked list from a memory pool still sounds good though, since that could also be a way of getting the accurate mode for GPUs.

Rainer Trummer (aliasguru) awarded a token.Jul 20 2018, 6:43 PM

Rainer Trummer (aliasguru) added a subscriber: Rainer Trummer (aliasguru).

In D3538#81848, @Stefan Werner (swerner) wrote:

I think I found a way to write id passes using atomics.

That seems like it would work, as long as there are enough free ID slots. If there are not enough it's random which object ID get skipped, but I guess that is acceptable for now.

Without resorting to dynamic memory allocations inside the kernel or grossly over allocating memory, there will always be a chance of randomly skipped data.

In D3538#81903, @Stefan Werner (swerner) wrote:

Without resorting to dynamic memory allocations inside the kernel or grossly over allocating memory, there will always be a chance of randomly skipped data.

I mean random in that the results could be different between renders, so the result is not exactly repeatable. A solution could be to write the minimum sample number next to each ID and prioritize lower samples, but I'm not sure it's worth it in practice.

Tanguy Weyland (Tanguy) added a subscriber: Tanguy Weyland (Tanguy).Jul 25 2018, 4:02 PM

Manuel Albert (creamsurfer) added a subscriber: Manuel Albert (creamsurfer).Jul 27 2018, 6:43 AM

Merge branch 'master' of git.blender.org:blender into cycles_cryptomatte
Merge branch 'master' of git.blender.org:blender into cycles_cryptomatte
Cycles: Implemented full sorting for Cryptomatte layers for all possible renderign devices.
Cycles: Refactoring to remove duplicate Cyryptomatte id pass code.
Cycles: Addressed some of Sergey's comments on D3538
Cycles: Stochastic cryptomatte writing now adds coverage to last ID when it runs out of slots.

Addressed some of Sergey's comment.s

Cycles: A little refactor of the cryptomatte ID pass writting to reduce #ifdef clutter
Cycles: Removed PathState::matte_weight, using throughput instead for Cryptomatte

Bruno Ortolland (Noss) awarded a token.Aug 3 2018, 9:37 PM

Steffen Dünner (SteffenD) awarded a token.Aug 3 2018, 10:08 PM

Vitor Balbio (vitorbalbio) awarded a token.Aug 4 2018, 12:11 AM

Vitor Balbio (vitorbalbio) added a subscriber: Vitor Balbio (vitorbalbio).

Tanguy Weyland (Tanguy) awarded a token.Aug 5 2018, 4:18 PM

Andrew Charlton (Scaredyfish) awarded a token.Aug 9 2018, 7:02 AM

Yegor (Yegor) added a subscriber: Yegor (Yegor).Aug 9 2018, 2:25 PM

Some more inline comments.

intern/cycles/kernel/kernel_id_passes.h
33 ↗	(On Diff #11499)	Since CAS always returns the old value, we could actually detect the case that another thread already allocated this slot for the same ID here and get rid of the "--slot;" part.
48 ↗	(On Diff #11499)	For consistency, I'd prefer to also test for slot ID == ID_NONE here.
intern/cycles/kernel/kernel_passes.h
296	Could we move `2 * kernel_data.film.cryptomatte_depth` into the macro? It's always the same and it would make this part easier to read.
intern/cycles/kernel/kernel_path_branched.h
560	I think this change can be removed?
intern/cycles/kernel/kernels/cuda/kernel.cu
54 ↗	(On Diff #11499)	Why is the sorting part of the kernel? I'd prefer to just move it to `buffers.cpp`, that would simplify the code a lot. Also, sample is uninitialized if `work_index >= total_work_size`.
intern/cycles/render/coverage.cpp
32	The code here could be deduplicated by having a wrapper function (or maybe function pointer?) that calls either `flatten_buffer` or `sort_buffer`.

Manu Järvinen (maxon) awarded a token.Aug 14 2018, 9:08 PM

Lukas Stockner (lukasstockner97) added a dependent revision: D3606: Image Editor: Add option for showing a matte based on Cryptomatte render passes.Aug 15 2018, 12:10 AM

Lukas Stockner (lukasstockner97) added a dependent revision: D3607: Cycles: Implement render passes that contain all lighting from lights in a specified group.Aug 15 2018, 2:33 AM

S J Bennett (quollism) added a subscriber: S J Bennett (quollism).Aug 15 2018, 1:38 PM

Cycles: More Cryptomatte code review changes.
Cycles: Skip calls to CPU kernel coverage buffer when accurate cryptomatte layers are turned off.

Harbormaster completed building B1920: Diff 11613.Aug 17 2018, 8:51 PM

Stefan Werner (swerner) marked 2 inline comments as done.Aug 17 2018, 10:17 PM

Stefan Werner (swerner) marked 2 inline comments as done.

Looks good to me except for one detail.

intern/cycles/kernel/kernel_id_passes.h
48 ↗	(On Diff #11613)	This should check `.x` instead of `.y` I guess? In that case, the assert needs to be changed to `.y == 0.0f`.

Cycles: Fixed typos in Crytpomatte code.

Bartosz Moniewski (monio) awarded a token.Aug 20 2018, 10:54 PM

Fixed indentation.

Harbormaster completed building B1928: Diff 11634.Aug 21 2018, 2:17 PM

intern/cycles/blender/blender_sync.cpp
626	Narrowing from `int`. Is really needed to be `unsigned short` ?
intern/cycles/kernel/kernels/cuda/kernel.cu
51–62 ↗	(On Diff #11634)	Move to a function, call from the `if()` statement above.
53 ↗	(On Diff #11634)	This is quite scary.
intern/cycles/kernel/split/kernel_buffer_update.h
83 ↗	(On Diff #11634)	Need update? Is being updated? A function pointer to function which updates the ray?
intern/cycles/render/buffers.cpp
385–388	Clearly, only scale `y` and `w`. Without comment ;)
intern/cycles/render/coverage.cpp
18–24	The convention is to: Have own include first. System includes. Rest of includes, in alphabetical order.
28	There is `pair` in `ccl` namespace, see `util_map.h`.
69	Either use `int` for `pixel_index` or do a cast of `tile.w` to`size_t` to make math happen in `size_t`. Currently it's happening in `int`.
102	Why this index is in `int`?
intern/cycles/render/coverage.h
36–38	Can we typedef `unordered_map<float, float>` for the clarity. Will shorten so much code!
intern/cycles/render/film.h
80	Make a named enum (ideally), and do not use `use_` for not-a-boolean.
81	Just int.
intern/cycles/util/util_murmurhash.cpp
24	Can we have this file more cycles-styled?

This revision now requires changes to proceed.Aug 21 2018, 5:16 PM

Cycles: Addressed Sergey's cryptomatte omments.
Cycles: Fixed OpenCL build

Harbormaster completed building B1951: Diff 11663.Aug 23 2018, 3:49 PM

Cycles: Code styling

Yegor (Yegor) removed a subscriber: Yegor (Yegor).Aug 23 2018, 11:01 PM

Cycles: Code styling
Merge branch 'cycles_cryptomatte' of github.com:skwerner/blender into cycles_cryptomatte

Some more updates, addressing Sergey's comments.

intern/cycles/kernel/kernels/cuda/kernel.cu
54 ↗	(On Diff #11499)	Moving it to buffers.cpp excludes it from GPU acceleration. Putting it in buffers.cpp isn't straight forward. RenderBuffers is agnostic of total sample count, and sorting every time get_pass_rect is called is wasteful. RenderTile knows about total sample count but is just a structure of data without methods. Turning it into its own RenderTile::Task (similar to denoising) seems like overkill to me.

Michael P. (forest-house) added a subscriber: Michael P. (forest-house).Aug 26 2018, 8:59 AM

Aditia A. Pratama (aditiapratama) added a subscriber: Aditia A. Pratama (aditiapratama).Sat, Oct 20, 7:35 PM

		Path
M		intern/cycles/blender/addon/engine.py (10 lines)
M		intern/cycles/blender/addon/properties.py (31 lines)
M		intern/cycles/blender/addon/ui.py (11 lines)
M		intern/cycles/blender/blender_object.cpp (17 lines)
M		intern/cycles/blender/blender_session.cpp (6 lines)
M		intern/cycles/blender/blender_sync.h (2 lines)
M		intern/cycles/blender/blender_sync.cpp (42 lines)
M		intern/cycles/device/device_cpu.cpp (8 lines)
M		intern/cycles/kernel/geom/geom_object.h (18 lines)
M		intern/cycles/kernel/kernel_globals.h (6 lines)
M		intern/cycles/kernel/kernel_passes.h (94 lines)
M		intern/cycles/kernel/kernel_path_branched.h (4 lines)
M		intern/cycles/kernel/kernel_path_state.h (1 line)
M		intern/cycles/kernel/kernel_path_surface.h (6 lines)
M		intern/cycles/kernel/kernel_shader.h (5 lines)
M		intern/cycles/kernel/kernel_types.h (26 lines)
M		intern/cycles/render/CMakeLists.txt (2 lines)
M		intern/cycles/render/buffers.h (4 lines)
M		intern/cycles/render/buffers.cpp (21 lines)
A	M	intern/cycles/render/coverage.h (46 lines)
A	M	intern/cycles/render/coverage.cpp (148 lines)
M		intern/cycles/render/film.h (13 lines)
M		intern/cycles/render/film.cpp (37 lines)
M		intern/cycles/render/object.h (1 line)
M		intern/cycles/render/object.cpp (5 lines)
M		intern/cycles/render/shader.cpp (4 lines)
M		intern/cycles/util/CMakeLists.txt (2 lines)
M		intern/cycles/util/util_atomic.h (37 lines)
A	M	intern/cycles/util/util_murmurhash.h (30 lines)
A	M	intern/cycles/util/util_murmurhash.cpp (129 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	11400	80373bc		Jul 12 2018, 10:37 PM	★	★
Diff 2	11401	80373bc	- Cycles: Code styling fixes for murmurhash.	Jul 12 2018, 10:58 PM	★	★
Diff 3	11402	80373bc	- Cycles: Fixed Cryptomatte pass names to match between Python and C++ code.	Jul 12 2018, 11:03 PM	★	★
Diff 4	11403	80373bc	- Fixed indentation.	Jul 12 2018, 11:07 PM	★	★
Diff 5	11413	80373bc	- Cycles: Addressed Lukas' comments for Cryptomatte output.	Jul 13 2018, 2:51 PM	★	★
Diff 6	11441	80373bc	- Cycles: Cryptomatte writing now uses atomics on GPUs to avoid race…	Jul 20 2018, 12:31 PM	★	★
Diff 7	11442	80373bc	- Cycles: Compile fixes for OpenCL kernel.	Jul 20 2018, 3:28 PM	★	★
Diff 8	11494	b59d85b	- Merge branch 'master' of git.blender.org:blender into cycles_cryptomatte	Aug 2 2018, 4:07 PM	★	★
Diff 9	11499	b59d85b	- Cycles: A little refactor of the cryptomatte ID pass writting to reduce…	Aug 3 2018, 9:28 PM	★	★
Diff 10	11613	b59d85b	- Cycles: More Cryptomatte code review changes.	Aug 17 2018, 8:51 PM	★	★
Diff 11	11622	b59d85b	- Cycles: Fixed typos in Crytpomatte code.	Aug 20 2018, 2:18 PM	★	★
Diff 12	11634	b59d85b	- Fixed indentation.	Aug 21 2018, 2:17 PM	★	★
Diff 13	11663	b59d85b	- Cycles: Addressed Sergey's cryptomatte omments.	Aug 23 2018, 3:49 PM	★	★
Diff 14	11668	b59d85b	- Cycles: Code styling	Aug 23 2018, 10:51 PM	★	★
Diff 15	11669	b59d85b	- Cycles: Code styling	Aug 24 2018, 12:00 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
8a84f53ff04c	d83ffdf0e940	6d63dbf1908e	Stefan Werner	Cycles: Code style, fixed typos moving murmurhash to its own cpp file.	Jul 20 2018, 12:25 PM
6d63dbf1908e	e4cd5686a17e	c41edf6c00f2	Stefan Werner	Cycles: Cryptomatte writing now uses atomics on GPUs to avoid race conditions.	Jul 20 2018, 12:01 PM
c41edf6c00f2	d7158add275d	33bc0902b043	Stefan Werner	Cycles: Addressed Lukas' comments for Cryptomatte output.	Jul 13 2018, 2:43 PM
33bc0902b043	42811e3a9c37	0fb4cb2bfca5	Stefan Werner	Fixed indentation.	Jul 12 2018, 11:07 PM
0fb4cb2bfca5	30bb0da8e992	9e53eb750096	Stefan Werner	Cycles: Fixed Cryptomatte pass names to match between Python and C++ code.	Jul 12 2018, 11:02 PM
9e53eb750096	0a0d76ddba90	cb582e31b5a3 0656f362511f	Stefan Werner	Merge branch 'cycles_cryptomatte' of github.com:skwerner/blender into… (Show More…)	Jul 12 2018, 10:57 PM
cb582e31b5a3	0a0d76ddba90	fd523041f48c	Stefan Werner	Cycles: Code styling fixes for murmurhash.	Jul 12 2018, 10:56 PM
fd523041f48c	8ea2b765725b	084b00e7db10	Stefan Werner	Cycles: Added Cryptomatte passes. (Show More…)	Jul 12 2018, 6:24 PM
0656f362511f	8ea2b765725b	084b00e7db10	Stefan Werner	Cycles: Refactored cryptomatte coverage.	Jul 12 2018, 6:24 PM
084b00e7db10	3b02e599ca4d	d4f27b960bf8	Stefan Werner	Cycles: More fixes for Cryptomatte	Jul 12 2018, 11:29 AM
d4f27b960bf8	edf872e27ad1	c36977652aa8	Stefan Werner	Cycles: Switched CPU cryptomatte to use unordered_map instead of map.	Jul 11 2018, 12:20 PM
c36977652aa8	0300a85880fd	80373bc4d2b4	Stefan Werner	Cycles: Initial addition of Cryptomatte ID writing.	Jul 11 2018, 12:00 AM

Status	Author	Revision
Needs Review	Lukas Stockner (lukasstockner97)	D3606 Image Editor: Add option for showing a matte based on Cryptomatte render passes
Needs Review	Lukas Stockner (lukasstockner97)	D3607 Cycles: Implement render passes that contain all lighting from lights in a specified group
Needs Review	Stefan Werner (swerner)	D3538 Cycles: Added Cryptomatte passes.

Cycles: Added Cryptomatte passes.
Needs ReviewPublic
Actions

Details

Diff Detail

Revision Contents

Diff 11441

intern/cycles/blender/addon/engine.py

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_session.cpp

intern/cycles/blender/blender_sync.h

intern/cycles/blender/blender_sync.cpp

intern/cycles/device/device_cpu.cpp

intern/cycles/kernel/geom/geom_object.h

intern/cycles/kernel/kernel_globals.h

intern/cycles/kernel/kernel_passes.h

intern/cycles/kernel/kernel_path_branched.h

intern/cycles/kernel/kernel_path_state.h

intern/cycles/kernel/kernel_path_surface.h

intern/cycles/kernel/kernel_shader.h

intern/cycles/kernel/kernel_types.h

intern/cycles/render/CMakeLists.txt

intern/cycles/render/buffers.h

intern/cycles/render/buffers.cpp

intern/cycles/render/coverage.h

intern/cycles/render/coverage.cpp

intern/cycles/render/film.h

intern/cycles/render/film.cpp

intern/cycles/render/object.h

intern/cycles/render/object.cpp

intern/cycles/render/shader.cpp

intern/cycles/util/CMakeLists.txt

intern/cycles/util/util_atomic.h

intern/cycles/util/util_murmurhash.h

intern/cycles/util/util_murmurhash.cpp

Cycles: Added Cryptomatte passes.Needs ReviewPublicActions

Details

Diff Detail

Revision Contents

Diff 11441

intern/cycles/blender/addon/engine.py

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_session.cpp

intern/cycles/blender/blender_sync.h

intern/cycles/blender/blender_sync.cpp

intern/cycles/device/device_cpu.cpp

intern/cycles/kernel/geom/geom_object.h

intern/cycles/kernel/kernel_globals.h

intern/cycles/kernel/kernel_passes.h

intern/cycles/kernel/kernel_path_branched.h

intern/cycles/kernel/kernel_path_state.h

intern/cycles/kernel/kernel_path_surface.h

intern/cycles/kernel/kernel_shader.h

intern/cycles/kernel/kernel_types.h

intern/cycles/render/CMakeLists.txt

intern/cycles/render/buffers.h

intern/cycles/render/buffers.cpp

intern/cycles/render/coverage.h

intern/cycles/render/coverage.cpp

intern/cycles/render/film.h

intern/cycles/render/film.cpp

intern/cycles/render/object.h

intern/cycles/render/object.cpp

intern/cycles/render/shader.cpp

intern/cycles/util/CMakeLists.txt

intern/cycles/util/util_atomic.h

intern/cycles/util/util_murmurhash.h

intern/cycles/util/util_murmurhash.cpp

Cycles: Added Cryptomatte passes.
Needs ReviewPublic
Actions