Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Group Reviewers

Cycles

Commits

rBSe58c6cf0c678: Cycles: Added Cryptomatte output.
rBe58c6cf0c678: Cycles: Added Cryptomatte output.
rC56bc0328c1ae: Added Cryptomatte output

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

Event Timeline

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

Sergey Sharybin (sergey) requested changes to this revision.Jul 20 2018, 12:19 PM

Sergey Sharybin (sergey) added a subscriber: Sergey Sharybin (sergey).

Sergey Sharybin (sergey) added inline comments.

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 remote builds in 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.

Stefan Werner (swerner) added inline comments.Jul 20 2018, 5:03 PM

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 child 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 child 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 remote builds in 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 remote builds in B1928: Diff 11634.Aug 21 2018, 2:17 PM

Sergey Sharybin (sergey) requested changes to this revision.Aug 21 2018, 5:16 PM

Sergey Sharybin (sergey) added inline comments.

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 remote builds in 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).Oct 20 2018, 7:35 PM

Brecht Van Lommel (brecht) accepted this revision.Oct 26 2018, 4:39 PM

This revision was not accepted when it landed; it landed in state Needs Review.Oct 28 2018, 10:52 AM

Closed by commit rBe58c6cf0c678: Cycles: Added Cryptomatte output. (authored by Stefan Werner (swerner)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Jonatan Mercado (jonimercado) awarded a token.Oct 29 2018, 6:20 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	★	★
Diff 16	12258	c0b3e3d	rBe58c6cf0c678849cf9c348a8df5e0ec24a6abd4d	Oct 28 2018, 10:37 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

Cycles: Added Cryptomatte passes.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

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.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

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.
ClosedPublic
Actions

Revision Contents
Changeset List