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

feat(jpeg): Support reading Ultra HDR images #4484

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

mugulmd
Copy link
Contributor

@mugulmd mugulmd commented Oct 8, 2024

Description

Initial feature request: #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:

  • I have read the contribution guidelines.
  • 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.)
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • 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).
  • 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.

Copy link

linux-foundation-easycla bot commented Oct 8, 2024

CLA Not Signed

src/jpeg.imageio/jpeg_pvt.h Outdated Show resolved Hide resolved
src/jpeg.imageio/jpeginput.cpp Outdated Show resolved Hide resolved
src/jpeg.imageio/jpeginput.cpp Outdated Show resolved Hide resolved
@mugulmd mugulmd force-pushed the jpeg-ultrahdr branch 3 times, most recently from ac8f6b7 to 5c0698e Compare October 10, 2024 14:48
@mugulmd
Copy link
Contributor Author

mugulmd commented Oct 10, 2024

@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 ?

@lgritz
Copy link
Collaborator

lgritz commented Oct 10, 2024

Images repo, please

src/jpeg.imageio/jpeg_pvt.h Outdated Show resolved Hide resolved
@lgritz
Copy link
Collaborator

lgritz commented Oct 18, 2024

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.

@mugulmd mugulmd marked this pull request as ready for review October 18, 2024 15:24
@mugulmd
Copy link
Contributor Author

mugulmd commented Oct 18, 2024

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 :)

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

We'll leave this pending the CLA and adding a proper test including source in OpenImageIO-images. When those items are completed, I think it's ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants