Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bundle OpenEXR and enable import & conversion plugins #1447

Merged
merged 11 commits into from
Aug 30, 2021
Merged

Bundle OpenEXR and enable import & conversion plugins #1447

merged 11 commits into from
Aug 30, 2021

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Aug 23, 2021

Motivation and Context

This PR enables OpenEXR that's needed for @bigbike's PBR IBL and HDR / depth image export. It adds support for importing and converting OpenEXR files with half-float (for color), float (for depth) and integer (for object ID) channels, including cubemaps and multiple mip levels per file.

Apart from that, the magnum-imageconverter tool got extended to make working with cubemap and multi-level EXR files easier. Running --info will give an overview of what's there:

$ magnum-imageconverter --info cube.exr
3D image 0: PixelFormat::RGB16F Vector(256, 256, 6)
$ magnum-imageconverter --info cube-mips.exr
3D image 0:
  Level 0: PixelFormat::RGB16F Vector(256, 256, 6)
  Level 1: PixelFormat::RGB16F Vector(128, 128, 6)
  Level 2: PixelFormat::RGB16F Vector(64, 64, 6)
  Level 3: PixelFormat::RGB16F Vector(32, 32, 6)
  Level 4: PixelFormat::RGB16F Vector(16, 16, 6)
  Level 5: PixelFormat::RGB16F Vector(8, 8, 6)
  Level 6: PixelFormat::RGB16F Vector(4, 4, 6)
  Level 7: PixelFormat::RGB16F Vector(2, 2, 6)
  Level 8: PixelFormat::RGB16F Vector(1, 1, 6)

The following will combine 6 2D cube map faces into a single 3D EXR file:

magnum-imageconverter --layers -c envmap=cube -- \
    +x.exr -x.exr +y.exr -y.exr +z.exr -z.exr cube.exr

This will make a file that contains several mipmap levels:

magnum-imageconverter --levels -c envmap=cube -D3 \
    cube-256.exr cube-128.exr cube-64.exr cube-mips.exr

And, as the inverse of above, this will extract a single level of a particular face back into a 2D image:

magnum-imageconverter cube-mips.exr --layer 2 --level 1 +x-128.exr

Please check out the full documentation for more information:

Open questions / TODOs:

  • OpenEXR does not support 8 bits per channel, only 16- and 32-bit floats, which means converting (LDR) JPEG images to a (HDR) EXR can't be done directly with magnum-imageconverter. I attempted to implement this as a convenience feature in the tool but abandoned the idea halfway through, as I don't feel it's important enough compared to my other tasks. Use imagemagick for now instead: convert image.jpg image.exr
  • OpenEXR requires CMake 3.12, while Habitat CIs run on 3.11 right now. Should we upgrade there?
  • Right now, OpenEXR is bundled and enabled always, since I don't know if it'll be an integral part of the PBR pipeline or something optional. (OTOH there's an option to use a system installation of it.) The goal of this PR is to prepare the groundwork for HDR additions to @bigbike's PBR pipeline, leaving the decision about EXR being optional or not for later.
  • I have no idea if the OpenEXR library builds on Emscripten it does, but do we actually want it enabled there (download sizes, etc.)?

How Has This Been Tested

Magnum changes tested locally and on my CIs. Habitat build fails due to the above-mentioned CMake requirement.

Types of changes

  • New feature (non-breaking change which adds functionality)

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 23, 2021
@Skylion007
Copy link
Contributor

We need to bump our minimum CMake for this PR.

Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either bump minimum CMake in this PR or get it working with the older version.

@erikwijmans
Copy link
Contributor

I'm fine bumping cmake. Since we support cmake installed via pip install cmake I don't see any reason why we can't bump it.

@Skylion007
Copy link
Contributor

One last spot:

cmake_minimum_required(VERSION 3.10)

Copy link
Contributor

@bigbike bigbike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

Thanks a lot for your STRONG support, @mosra.

@mosra
Copy link
Collaborator Author

mosra commented Aug 27, 2021

@Skylion007 do you think that change is needed? Because the submodule will enforce it anyway, and if the submodule isn't there (because OpenEXR becomes optional, or because it's disabled for Emscripten, or because the user relies on a system installation), then there's no point in enforcing 3.12.

@bigbike I'll leave the decision on whether make OpenEXR required or optional on you, as well as a merge of this PR. I'd suggest to base your HDR code off this branch and when you're done and it's clear whether we want to implicitly rely on OpenEXR or not, merge both PRs.

@Skylion007
Copy link
Contributor

@mosra Good point, forgot the submodule should enforce it. Feel free to revert it.

@bigbike
Copy link
Contributor

bigbike commented Aug 27, 2021

@mosra :

I'd suggest to base your HDR code off this branch and when you're done and it's clear whether we want to implicitly rely on OpenEXR or not, merge both PRs.

This suggestion is very thoughtful. Thank you.
If we make a decision now (e.g., making it optional), how hard is it to switch it to "required" in the future? I guess the overhead is not very big?
This PR has some other merits so I would recommend committing it in without waiting my other diffs.

@mosra
Copy link
Collaborator Author

mosra commented Aug 27, 2021

how hard is it to switch it to "required" in the future?

That direction is not hard. The overhead of having it optional is what's big. In other words, if I make this optional now, it'll require you to add some #ifdefs all around the code from the very beginning, invent fallback paths without EXR etc etc., and depending how deep you need the EXR integration to be, having it optional might as well turn out to be impossible. And that's what I don't know, I don't know your plans, which is why I left this decision up to you.

If you think the build time / binary size / conda build size overhead is not too big to be significant, the easiest option is to merge this as-is. I tried to look at the CIs to do at least a rough comparison, but it says this branch built in 6 minutes and master in 44, so I have no idea :)

@Skylion007
Copy link
Contributor

So I just tested it, and it looks like it increases compilation time by 20-30 seconds at most. We don't really care at all about binary size at all at the moment so I would just leave it enabled by default.

@mosra
Copy link
Collaborator Author

mosra commented Aug 30, 2021

Haha, everything is relative. 20-30 seconds is the time needed to compile the whole Magnum :)

Sure, if that's fine, feel free to press the merge button.

@Skylion007 Skylion007 merged commit d9d7e16 into master Aug 30, 2021
@Skylion007 Skylion007 deleted the exr branch August 30, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants