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

Initial Exif and XML metadata support for JPEG XL #4310

Merged
merged 7 commits into from
Jul 11, 2024

Conversation

ssh4net
Copy link
Contributor

@ssh4net ssh4net commented Jun 23, 2024

Description

Initial Exif and XML metadata support for JPEG XL.
EXIF - works and enabled by default
XML - works and enabled by default
IPTC - that part need some help. JPEG XL support XML based IPTC, but I do not get what IPTC create an OIIO
JBIG - not sure

new options:
jpegxl:use_boxes - export EXIF, XMP, IPTC, JBIG
jpegxl:compress_boxes - compress meta boxes (disabled for this moment by default)

jpegxl:exif_box = 1 (enabled)
jpegxl:xmp_box = 1 (enabled)
jpegxl:jumb_box = 0 (disabled)
jpegxl:iptc_box = 0 (disabled)

Tests

Not yet, but if we have I can add.

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • 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.

Signed-off-by: Vlad (Kuzmin) Erium <[email protected]>
@lgritz
Copy link
Collaborator

lgritz commented Jul 3, 2024

You're getting build failures on Mac because of an unused variable warning.

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.

I made a couple very minor style suggestions for clarity.

Also, there is a warning failure for clang related to not using the return value of save_metadata.

But other than those, LGTM!

Can you incorporate someting into the jxl tests so that the testsuite verifies writing and reading of metadata?

src/jpegxl.imageio/jxloutput.cpp Outdated Show resolved Hide resolved
src/jpegxl.imageio/jxloutput.cpp Outdated Show resolved Hide resolved
src/jpegxl.imageio/jxloutput.cpp Show resolved Hide resolved
Signed-off-by: Vlad (Kuzmin) Erium <[email protected]>
@ssh4net
Copy link
Contributor Author

ssh4net commented Jul 8, 2024

@lgritz I'll check how can add something to tests. But at this moment only writing was added.

I also need change a docs a bit

Signed-off-by: Vlad (Kuzmin) Erium <[email protected]>
Signed-off-by: Vlad (Kuzmin) Erium <[email protected]>
@ssh4net
Copy link
Contributor Author

ssh4net commented Jul 10, 2024

@lgritz don't know how to do a tests without implementing exif for import.
I'll probably add jpegxl import exif support, but later. Need to work on other project at this moment.

@lgritz
Copy link
Collaborator

lgritz commented Jul 10, 2024

@lgritz don't know how to do a tests without implementing exif for import.

No prob, we can just remember to add tests when the input support is added.

@lgritz
Copy link
Collaborator

lgritz commented Jul 10, 2024

@ssh4net This LGTM except for the CI failure that appears to not like 'const' in the struct definition for BoxInfo.

Also, would you mind rebasing on top of the current main just to be sure there aren't any conflicts?

After those two items, I'm good to merge this.

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!

@lgritz lgritz merged commit 4185ec9 into AcademySoftwareFoundation:master Jul 11, 2024
26 checks passed
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.

4 participants