Cryptomatte always needs full float precision. This patch will allow for that even in 16bit sequences
Needs RevisionPublic
Actions

Authored by Evan Ryan (evantryan) on Jul 28 2021, 10:45 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Jeroen Bakker (jbakker)

Summary

This keys off of the pass name to decide if channel should be 32 bit.It works similarly to the way Mist and Depth are handled.

It'll allow for one multilayer exr sequence output with a working cryptomatte while also using less space for channels that don't need full float

If keying off the channel name isn't ideal, maybe rp could have some sort of type attribute that could be used to decide what gets full float.

The current limitations of this are in the compositor. I have been unable to find a channel name in OutputOpenExrMultiLayerOperation to use.

I don't think this needs any user interface. The documentation on Cryptomatte should warn that those passes from a render are always full float.

I've successfully tested the resulting exrs in nuke and blender's compositor.

Diff Detail

Repository

rB Blender

Branch

float_cryptomatte (branched from master)

Build Status

Buildable 16104
Build 16104: arc lint + arc unit

Event Timeline

Evan Ryan (evantryan) requested review of this revision.Jul 28 2021, 10:45 PM

Evan Ryan (evantryan) created this revision.

Harbormaster completed remote builds in B16103: Diff 40092.Jul 28 2021, 10:46 PM

Evan Ryan (evantryan) retitled this revision from Cryptomatte always needs full float precision. This keys off of the pass name to decide if channel should be 32 bit. It works similarly to the way Mist and Depth are handled. It'll allow for one multilayer exr sequence output with a working... to Cryptomatte always needs full float precision. This patch will allow for that even in 16bit sequences.Jul 28 2021, 10:47 PM

Evan Ryan (evantryan) edited the summary of this revision. (Show Details)

reverted .editorconfig to not be in diff

Harbormaster completed remote builds in B16104: Diff 40093.Jul 28 2021, 10:54 PM

Evan Ryan (evantryan) edited the summary of this revision. (Show Details)Jul 28 2021, 11:01 PM

Evan Ryan (evantryan) added a reviewer: Brecht Van Lommel (brecht).Jul 28 2021, 11:44 PM

Hi and thx working on this!

There is a task for this, see T88049: Cryptomatte EXR Output Bit Depth should always be 32bit..
Note the comments made there already though:

In T88049#1156249, @Jeroen Bakker (jbakker) wrote:

Now that we have the metadata we can make this easier to detect. Haven’t looked at the technicalities of this but would be a good addition

In T88049#1159452, @Brecht Van Lommel (brecht) wrote:

I agree metadata would be better than, doing it based purely on pass names is not going to work in general.
But even that is not possible to make fully automatic, you can't always guess form the node setup if it's color or data. So every input still needs manual control I think, though it could auto detect by default for the most common cases.

Philipp Oeser (lichtwerk) edited reviewers, added: Jeroen Bakker (jbakker); removed: Campbell Barton (campbellbarton).Jul 29 2021, 12:17 AM

Yeesh! I'm not sure how I missed that task. Thank you for pointing me in that direction.

I should preface the following by saying that I'm still very new to both C++ in general and blender's source. From what I could tell, exr writing takes a different route depending on whether it comes from the compositor's File Output node or directly from a render. Since the render result (not compositor) is coming from render_result.c, wouldn't it be safe to use 'Crypto' string as something to decide if float precision is needed? Mainly because the pass names at that point aren't user editable.

Passes coming of the compositor could then be handled differently. Perhaps using the existence of metadata as the thing to decide full float requirement?

Evan Wilson (EAW) awarded a token.Aug 2 2021, 4:23 AM

Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).

Jeroen Bakker (jbakker) requested changes to this revision.Aug 2 2021, 9:23 AM

Jeroen Bakker (jbakker) added inline comments.

source/blender/render/intern/render_result.c
983	In stead of checking the name we should add a flag to the render pass that it supports half. Render engines should set this flag when they create render passes `RE_engine_register_pass` / `EEVEE_cryptomatte_update_passes`. This flag should also be set when creating the cryptomatte passes in cycles. `engine.py#register_passes` this requires an additional parameter in `register_pass` function in `RNA_render.c` (this files maps to `RE_engine_register_pass` mentioned above).

This revision now requires changes to proceed.Aug 2 2021, 9:23 AM

Needing a Cryptomatte exception for this is not ideal, but it seems indeed the Cryptomatte spec specifies RGBA channel names, so checking for XYZW like we do for other data passes does not work.

source/blender/render/intern/render_result.c
983	Getting it from the render passes makes sense, however for the compositor File Output node we don't have that information, so we couldn't apply the same fix there. Perhaps the most general fix would be to gather the cryptomatte pass names from the metadata, and then check against that. It should be possible to write a function that quickly extracts the layer names from `StampData` (see `CryptomatteStampDataCallbackData::extract_layer_names` for reference). And then check against the list of cryptomatte layer names to see if the layer needs to be saved as full float.

I've started working on the revision @Jeroen Bakker (jbakker) suggested. It's not working just yet. Everything checks out beautifully in engine.c RE_engine_register_pass but for some reason every pass has the half_float_allowed flag set to false by the time it gets to render_result.c IMB_exr_add_channel

I'm hoping I'm just doing something silly and someone can point out what that is. Assuming we can get this working the (STR_ELEM(rp->chan_id, "RGB", "RGBA", "R", "G", "B", "A")); check for the z/mist passes could be removed.

Passes from the compositor would still need to get worked out. Checking against the metadata isn't a bad idea, but it might be nice to pass this flag along somehow so z/mist could also benefit.

Harbormaster completed remote builds in B16273: Diff 40423.Aug 6 2021, 8:49 PM

Here's a couple blender files I've been using for testing. I'm using oiiotool --info -a -v out.0001.exr to check channel bit depth.

test_exr_eevee.blend935 KBDownload

test_exr_cycles.blend934 KBDownload

The solution that @Brecht Van Lommel (brecht) proposed is easier to implement and the API doesn't need to be changed. It would be better to follow that path.

Jeroen Bakker (jbakker) requested changes to this revision.Aug 9 2021, 9:02 AM

This revision now requires changes to proceed.Aug 9 2021, 9:02 AM

Tolkfan (Tolkfan) awarded a token.Aug 4 2022, 8:19 PM