OpenImageIO icon indicating copy to clipboard operation
OpenImageIO copied to clipboard

feat(jpeg): Support reading Ultra HDR images

Open mugulmd opened this issue 1 year ago • 5 comments

Description

Initial feature request: https://github.com/AcademySoftwareFoundation/OpenImageIO/issues/4424

Add support in the jpeg plugin for reading Ultra HDR images using the reference codec libultrahdr: https://github.com/google/libultrahdr

Tests

Images used for testing during development: https://github.com/MishaalRahmanGH/Ultra_HDR_Samples

Checklist:

  • [x] I have read the contribution guidelines.
  • [x] I have updated the documentation, if applicable. (Check if there is no need to update the documentation, for example if this is a bug fix that doesn't change the API.)
  • [x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • [x] If I added or modified a C++ API call, I have also amended the corresponding Python bindings (and if altering ImageBufAlgo functions, also exposed the new functionality as oiiotool options).
  • [x] My code follows the prevailing code style of this project. If I haven't 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.

mugulmd avatar Oct 08 '24 19:10 mugulmd

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: mugulmd / name: Loïc Vital (b5c78cafccdb62af914cac51604745f6be5019b2, 8e6ed95f4e29a2b8954e17b64fb47c6b225e653c, 16d1d6218d7d513cb961293ec7438a4c4c5c3751, bb9018c2d7bef662be104110d79a6c756079d5a2, 52295c110021c98046f2fab6a94b7295355a1d0d, a792186522fd9c4bca47cf0592be241abc4dbddc, 09962ea060ecb7d8bc985889f54735e7ca55e58e, 14610042e0998fb709c764e6a2d19b5ed2160a04, c9de51080ac6b5f2b50e1bc65ddd8d57923df26e, 39185b49b9066ebc980998859dbbc835337972b4, 56b26472a2f7707ca7d01ac768b41f5ac2b4efc4, 31943e5126f4f485e1e565de5a2b97498e3f69d5, cdf5c3dcacb333d49b15322aa86544f5b464c4d4, 10706fc472eca0788b72bda9392718fd2dcd24c7, 45038c549673d13b771bf2ba3b253288fd1174a3, 3061eeb5f34ffa70f4503c39799a70e7fb4c2441, 81f395652b37bdf004baccb11775788dfad5e399, 3d8bb6a9a1e0dbd11cc4a9a6c8c779f6ce4bf3cb)

@lgritz quick question: I will need to add at least one Ultra HDR image for the test suite. The ones I use for testing locally weigh a bit more than 2M. Should I add them directly to the OpenImageIO repo, or should I create a PR in OpenImageIO-images to add them there ?

mugulmd avatar Oct 10 '24 17:10 mugulmd

Images repo, please

lgritz avatar Oct 10 '24 19:10 lgritz

What's happening with icc is that the libuhdr's build is passing parameters to the compiler that it doesn't recognize. Its build system probably assumes that if it's building on Linux, it must be either gcc or clang. But icc doesn't take those particular arguments.

I think the easiest thing is to just set the icc test (in ci.yml) to disable the use of libuhdr by adding to its setenvs: the extra definition DISABLE_libuhdr=1. The icc compiler is basically discontinued, so it's really not important to test that particular case with libuhdr support. It's not worth the trouble to try to fix it for an optional dependency.

lgritz avatar Oct 18 '24 03:10 lgritz

Once I get the CLA authorized by my company I'll be able to merge the PR containing the test image and then I'll re-introduce the test case in the test suite :)

mugulmd avatar Oct 18 '24 15:10 mugulmd

I think the libheif failures are unrelated to this PR. Just ignore those for the moment, I'll address them separately.

lgritz avatar Nov 04 '24 19:11 lgritz

It seems that the test I added for reading Ultra HDR images is not passing in the CI for some reason. I just checked it's passing on my local setup, so it must be an issue with how we build/link libultrahdr. I'll try to fix that :)

mugulmd avatar Nov 04 '24 19:11 mugulmd

Just out of curiosity, what was the fix that addressed the failed CI tests?

lgritz avatar Nov 05 '24 20:11 lgritz

You have just one test (icx) failing. Once that is fixed, I think this is ready to get final review and be merged.

lgritz avatar Nov 05 '24 20:11 lgritz

@lgritz it seems libuhdr had trouble locating the jpeg library. I think (but I'm not 100% sure) that it comes from the find_package(JPEG) instruction in libuhdr's CMakeLists.txt, which will redefine the JPEG_INCLUDE_DIRS and JPEG_LIBRARIES variables, and make them point to a different JPEG install (as it seems most of the containers used in the CI already have a JPEG install by default, but we're trying to use the jpeg-turbo that we manually installed). By looking into CMake's official FindJPEG.cmake, I noticed that the JPEG_INCLUDE_DIR and JPEG_LIBRARY variables are not necessarily redefined, therefore we can use them to point to the jpeg-turbo install, and pass them to libuhdr's build process. I also noticed that something similar was done for libTIFF, so I figured this might be a good idea, and apparently I got lucky :)

mugulmd avatar Nov 05 '24 21:11 mugulmd

Hey @lgritz I just did a few checks on the CI and it seems the icx test is failing only because libuhdr gets compiled with icx, so it seems to be a compiler issue. I have to admit I don't know much about icx, so can I suggest we override the compiler with g++ for the moment on that specific CI run ? I see we're already doing that for OCIO. (we can also create an issue on the libuhdr repo, it's possible they never tested compiling with icx and there might be some extra work to do on their side to support it properly)

mugulmd avatar Nov 06 '24 13:11 mugulmd

so can I suggest we override the compiler with g++ for the moment on that specific CI run ?

Yes, definitely! The point of the icx test is to make sure OIIO builds and runs correctly with that compiler. It's not our responsibility to make sure the dependencies also do so. If there's a problem with any dependency and a specialty compiler, we should just switch to building that dependency itself with the standard system compiler (gcc in this case) and move on.

lgritz avatar Nov 06 '24 16:11 lgritz

Alright @lgritz I think we're good to go. Thanks for the review(s) :) If there's anything more you'd like to add don't hesitate !

mugulmd avatar Nov 06 '24 18:11 mugulmd

There's still quite a few features from libuhdr we could expose in oiio, but yeah that felt like a pretty good starting point !

mugulmd avatar Nov 06 '24 21:11 mugulmd