Add htj2k as a compression option for the OpenEXR plugin in OIIO
#4785
Conversation
src/openexr.imageio/exrinput.cpp
Outdated
| case Imf::B44A_COMPRESSION: comp = "b44a"; break; | ||
| case Imf::DWAA_COMPRESSION: comp = "dwaa"; break; | ||
| case Imf::DWAB_COMPRESSION: comp = "dwab"; break; | ||
| case Imf::HT256_COMPRESSION: comp = "htj2k"; break; |
There was a problem hiding this comment.
You're going to need some sort of guard here to prevent it from being a compiler error when using older versions of the library. We used to have that for DWA compression, though we eventually removed the guards once our minimum supported OpenEXR version was high enough to be guaranteed to have those symbols.
Maybe something like
#if OPENEXR_CODED_VERSION >= 30400
Though that wouldn't cover the case of the current "main" if we haven't flipped the version number yet.
Did the OpenEXR htj2k patch add some kind of #define in its headers that would give us an easy way to check if those symbols are going to be defined?
There was a problem hiding this comment.
I contemplated on this last night too.
Now I'm inclined to not do this in OIIO until a bit later, and do this patch -- at the same time bump OIIO's required EXR version to 3.4?
HT2K is a new codec and maybe people will discover issues upon it being officially released by EXR upon 3.4; I think it is okay for OIIO to run "behind" exr a bit, and support this when we're confident that EXR 3.4 is running without problem for those low level developers who can/need to use EXR directly. At that point we'll open this feature on OIIO for the pipeline developers.
Maybe we can hang this PR as draft here for a while for early adopters. I'll keep rebase it from time to time.
Do we need to add a constant in OIIO python binding somewhere too?
There was a problem hiding this comment.
Our policy is to support OpenEXR versions up to at least 3 years back. It will be a long time before we can enforce openexr 3.4 as a minimum.
There was a problem hiding this comment.
We should handle this the same way as we have always handled other newly introduced features:
- Build against any version of OpenEXR (within reason, but going back at least as far as what VFX Platform said 3 years ago).
- Identify some preprocessor symbol or version in the OpenEXR header that accurately identifies whether the feature is available so that we don't have a broken build. If no such identifiable symbol exists, we should add it on the OpenEXR file, or we should bump the reported version in OpenEXR main now, rather than only upon the immediate lead-up to release in the fall.
- Use
#ifguards on the OIIO side when behavior needs to change for differing openexr versions, and when OIIO requests a feature from a version that doesn't support it, make sure we are giving appropriate error messages that make it clear that this is the reason.
We used to have exactly this scheme around DWAA and DWAB compression methods, but the extra logic was eventually removed when our minimum openexr was up to where DWA compression was added.
There was a problem hiding this comment.
Thanks for the feedback. I have edited the PR to include the macro-based switch.
I have done some preliminary testing and it seems to be working with the current EXR main tip. Also works with an older release which doesn't have the macro defined.
I'll do a bit more testing tomorrow.
Signed-off-by: lji <[email protected]>
Signed-off-by: lji <[email protected]>
lji-ilm
changed the title
htj2k as a compression option for the OpenEXR plugin in OIIO
|
Before this is ready to mark as non-draft, can you also please amend builtinplugins.md where there is a table or list of which compression methods we support? Thanks. |
…he exr plugin on OIIO. Signed-off-by: lji <[email protected]>
Thanks for the feedback. I added "htj2k" as one of the options for EXR compression in the doc. Here is a test sphinx rendering of the edited passage.
I also examined the python binding layer; however, as that layer is format agnostic, I don't think any change is required. Let me know if we should write a more detailed explanation on exr's htj2k option. |
It might be nice to point out that htj2k is only supported for OpenEXR >= 3.4. But other than that, this all looks good to me. |
Signed-off-by: lji <[email protected]>
Thanks for the feedback! I've updated the rst file with a note about this. Test sphinx rendering below: |
…wareFoundation#4785) After adding the new `htj2k` compression codec to OpenEXR, here we enable it in the OIIO stack so it will be recognized by the library and binary tools, such as using `oiiotool` to directly change compression type into htj2k. --------- Signed-off-by: lji <[email protected]>
…wareFoundation#4785) After adding the new `htj2k` compression codec to OpenEXR, here we enable it in the OIIO stack so it will be recognized by the library and binary tools, such as using `oiiotool` to directly change compression type into htj2k. --------- Signed-off-by: lji <[email protected]>
2 participants
Description
After adding the new
htj2kcompression codec to OpenEXR, here we enable it in the OIIO stack so it will be recognized by the library and binary tools, such as usingoiiotoolto directly change compression type into htj2k.EXR side is planning to do a round of renaming of enum names (AcademySoftwareFoundation/openexr#2048), and therefore this MR is marked as Draft for visibility for now. The enum names should be changed to
htj2kname after 2048 on the EXR side goes into the mainline.I'm also trying to study if more changes on the OIIO side is required to enabled the new
htj2kcodec throughout the OIIO tool chain... suggestions welcome.Tests
Draft MR, more test will be followed up.
Does this need a new test?
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.