Multiview OpenEXR save/load refactor and fixes.
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Oct 19 2017, 4:49 AM.

Details

Summary

Multiview EXRs are now always handled as multilayer internally, significantly
reducing the amount of code. The channels names that end up in the OpenEXR file
are intended to remain the same as before.

Also fixes two bugs:

  • Saving a multiview render from the image editor gave invalid files.
  • Loading multiview images with a single view from other software failed.

Diff Detail

Repository
rB Blender

Loading multiview images with a single view from other software failed.

I understand that you can't share the file, but how were the channels in this case? Can you share the output of exrheader FILE.exr for this image?

Your file breaks my 2.79 release entirely :)
I will test your patch first thing tomorrow morning just to double-check. Particularly for non-multiview cases, to make sure things are still 100%.

source/blender/render/intern/source/render_result.c
840

-BLI_listbase_count
+BLI_listbase_count_ex(&rr->views, 2)

856

-if( +if (

864

code style: multi-line if should have "{" in a new line. And -if( +if (.

916–917

-if( +if (

957

-if(
+if (

Dalai Felinto (dfelinto) requested changes to this revision.Oct 20 2017, 2:52 PM

You are introducing a regression for multi-layer Multi-View files that are rendering only one eye.
For example:

When rendering (-b FILE.blend -f 1) the file above with 2.79 and using exrheader we get:

multiView (type stringvector): "right"

Which is not there with your patch.

As for the Leaves.exr it's a file that has both views (and your patch fix it indeed). But you originally reported a file that had only one view and was Multi-View. I'm insisting on this because maybe we should using the "multiView" header info also for the mono-view (left or right only) single layer exr, which we are not at the moment.

This revision now requires changes to proceed.Oct 20 2017, 2:52 PM

Thanks for reviewing!

When rendering (-b FILE.blend -f 1) the file above with 2.79 and using exrheader we get:

multiView (type stringvector): "right"

Ok, I'll change that, this comment threw me off.

/* if rendered only one view, we treat as a a non-view render */

As for the Leaves.exr it's a file that has both views (and your patch fix it indeed). But you originally reported a file that had only one view and was Multi-View.

I think the issue specifically happens when there is one view per part. Leaves.exr has two parts with each one view, and the problematic file I have here has one part with one "main" view.

I'm insisting on this because maybe we should using the "multiView" header info also for the mono-view (left or right only) single layer exr, which we are not at the moment.

Yes, there is an inconsistency where (without this patch) we save multiView header information for mono-view renders, but then discard it when loading back the .exr. I think we should probably preserve that information for both save and load.

"/* if rendered only one view, we treat as a a non-view render */"

Right. But in this case I was referring to when we were not rendering a "mono" view, which is exactly this scenario. Misleading logic/comments are my bad though ;)

Now that I think of it, I think I left the single-layer exr without the view tag to keep it in compliance with old exr readers entirely. I may be wrong here but I think we were even using a different library to write these images back then. Anyways, +1 for including single view name header info for single-layer exrs as well.

Addressed code style comments, and always save/load multiview headers
if there are one or more views.

Also fix for crashes loading multipart and different data/display window
EXR files, introduced in the multiview commit. For example this file:
https://github.com/openexr/openexr-images/blob/master/Beachball/multipart.0001.exr

Now loading all test openexr-images at least don't crash or hang. A few
still fail (issues with multipart or unkown channels), but no worse than
before.

Single-layer is perfect now. For multi-layer you introduced a few changes. They may be by design, but I would like to hear from you on these, so they would at least be mentioned in the commit log:

(1) Combined channel is no longer exported (we still export Combined.Combined, but not RenderLayer.Combined).
This is valid for files without "Views" as well as with "Views".

(2) The main "view" can be suppressed from the channel names. In 2.79 I was doing it only for single-view multi-view files. This was a more serious concern back when Multi-View was introduced, so that Blender Multi-View files would open in prior Blenders without any regressions. For now I think it's fine to have explicit Layer.Pass.view.channel for any multi-layer file, since Blender multi-layer exr is its own thing anyways.

Leave view name out of channel name when saving as Individual.

(1) Combined channel is no longer exported (we still export Combined.Combined, but not RenderLayer.Combined).
This is valid for files without "Views" as well as with "Views".

It's the other way around I think, this patch exports RenderLayer.Combined but no longer Combined.Combined? Combined.Combined was introduced in 2.75 along with the multiview changes, but seems to just duplicate RenderLayer.Combined for one of the render layers. What was the purpose of that?

(2) The main "view" can be suppressed from the channel names. In 2.79 I was doing it only for single-view multi-view files. This was a more serious concern back when Multi-View was introduced, so that Blender Multi-View files would open in prior Blenders without any regressions. For now I think it's fine to have explicit Layer.Pass.view.channel for any multi-layer file, since Blender multi-layer exr is its own thing anyways.

I guess you are referring to saving as "Individual"? I've updated the patch to leave out the view name from the channel name in that case again, might as well leave it unchanged.

Combined.Combined was introduced in 2.75 along with the multiview changes

Oh ouch. You are right then, no changes to non-multiview EXR should have been introduced, my bad.

Patch is good for me (didn't test latest changes, but trust you did). Thanks for it, and sorry for the pedantic review ;)

This revision is now accepted and ready to land.Oct 20 2017, 9:26 PM
Brecht Van Lommel (brecht) planned changes to this revision.EditedOct 20 2017, 9:55 PM

Actually it seems Combined.Combined is the result of compositing when that's used. In 2.74 that was called Composite.Combined and only saved when using compositing. I'll need to do some more changes to get that working again.

This revision was automatically updated to reflect the committed changes.