feat(jpeg): support Ultra HDR output - #5335
Conversation
|
Hi, @mugulmd, just a reminder that this is still marked as a draft, needs a DCO, and has some clang-format fixes. I think that the "hobbled" failure is unrelated and has been separately fixed, should be fixed if you rebase this on top of the current main. |
|
Hi @lgritz yep sorry I just reached the stage of "this works on my machine, let's make a draft PR and finish this later" and I've been procrastinating a little bit 😅 I have some spare time in the days to come, I'll finish this properly! |
|
Can you rebase on top of current main to fix the apparent conflict in buildinplugins.rst? I wonder if the conflict is related to the several places where you seem to have changed whitespace, probably something your editor or pre-commit hooks did automatically? |
|
@lgritz all done! I found the culprit: Zed (which I use as editor) automatically removes trailing whitespaces on save. Should be good now 🙂 |
lgritz
left a comment
There was a problem hiding this comment.
This is looking really good!
Two minor design issues I commented on for us to discuss and possibly alter, but other than those, this all is great.
| return (feature == "exif" || feature == "iptc" || feature == "ioproxy"); | ||
| return (feature == "exif" || feature == "iptc" || feature == "ioproxy" | ||
| #if defined(USE_UHDR) | ||
| || feature == "uhdr" |
There was a problem hiding this comment.
I'm not crazy about "uhdr" here as a jpeg-specific query -- the supports() is supposed to be standard tokens that could apply to lots of formats. But I think it totally makes sense for an app to want to know if a given file format can support HDR images, so I would be in favor of:
- this being renamed to "hdr"
- documented in imageio.h where we explain what all the accepted ImageOutput::supports() tokens are
- added to other formats that support HDR output
There was a problem hiding this comment.
Actually I preferred to remove this for now as this is out-of-scope for this PR. I'll be happy to implement this in a follow-up PR though :)
| if (m_spec.format != TypeDesc::HALF && m_spec.format != TypeDesc::FLOAT) { | ||
| errorfmt( | ||
| "JPEG Ultra HDR output requires a half or float pixel data type, " | ||
| "not \"{}\"", | ||
| m_spec.format); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I wonder if the right thing here, when handed a request to output a data type that does not require HDR, it just to silently fall back to writing a normal JPEG, rather than a hard error?
I think the usual OIIO way is "the app doesn't necessarily have to know details of the file format, they make their request, and we try to fulfill it as best as we can, only failing if there is no way to make a good guess about 'what they probably want.'" I think all the writers are robust to being asked to write data types they don't support, they just fall back to a type that they do support that minimizes loss of precision versus the original request. This is what allows oiiotool in.fmt1 -o out.fmt2 to work in a common sense way without really caring about whether every data type that fmt1 supports is also supported in fmt2 and forcing the user or calling app to have to handle every kind of mismatch. If uint8 data is read from a jpeg and written to exr, we just silently write half, we don't refuse just because exr doesn't support uint8.
There was a problem hiding this comment.
Completely agreed, I got used to write "agressive" code because dealing with errors makes everything much easier to debug as a developer, but this is not ideal for user experience.
I updated the code so that any failure to setup the Ultra HDR output falls back to regular JPEG :)
(this means failure due to data type AND failures due to color space)
Signed-off-by: Loïc Vital <mugulmotion@gmail.com>
… flags Signed-off-by: Loïc Vital <mugulmotion@gmail.com>
Signed-off-by: Loïc Vital <mugulmotion@gmail.com>
Signed-off-by: Loïc Vital <mugulmotion@gmail.com>
Signed-off-by: Loïc Vital <mugulmotion@gmail.com>
|
Merged! As food for thought, I wonder what it would need to take (technically? culturally?) before having to set the "jpeg:uhdr" hint attribute on each output starts to seem like a burden? Is it worth having a global Will it ever be wise to have the default be "always write a jpeg with uhdr if handed half or float data and libultrahdr was used at build time?" IIRC, a reader not aware of uhdr will simply read such a file in LDR mode and still get a usable image, right? |
|
That's exactly right! So in theory it should be always safe to shoot for uhdr because:
However it seems that I'm going on vacation but after that I can definitely draft up a complete overview of what |
|
OIIO 3.1 added the ability to read UHDR in JPEG. Now for 3.2 we have the ability to write it to JPEG -- when asked. That's already a step forward, and lets us evaluate it, and we can always change future versions to do that by default as well as to add the capabilities to other formats. There's no reason to rush further changes. Enjoy your vacation! |
|
Nice work! Writing Ultra HDR images is something Blender users have requested, though I think mainly if it supports supplying separate SDR and HDR graded images, so that unlike other image formats there is more control over how it displays on SDR and HDR monitors. And the HDR would probably be in PQ or HLG. I added to the list in #4980 as a personal reminder, and might look into it one day if no one does before me, though probably not anytime soon. |
Assisted-by:
claude-code/opus-5Description
Related to #5229.
In its current state, OpenImageIO only supports reading Ultra HDR images, and does not support writing such images.
This feature has been requested because it enables converting an HDR image from specialized formats such as OpenEXR to widely-supported and easily displayable/exchangeable JPEG files.
libultrahdr(the codec we use for Ultra HDR) has plenty of options but we only use a restricted subset for simplicity, which leads to the following constraints:oiio:ColorSpacemust be one oflin_rec709_scene,lin_p3d65_scene,lin_rec2020_scenebecause it needs a corresponding "Ultra HDR color gamut".Also, in order to trigger the Ultra HDR output path, the incoming image must have the
jpeg:ultrahdrmetadata set to any non-zero integer.Tests
3 tests have been added to
jpeg-ultrahdr:Checklist:
and if I used AI coding assistants, I have an
Assisted-by: TOOL / MODELline in the pull request description above.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.