Skip to content

Conversation

@TodicaIonut
Copy link
Contributor

Description

After adding the new zstd 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 zstd.

add support zstd a new compression (AcademySoftwareFoundation/openexr#1604)

I'm also trying to study if more changes on the OIIO side is required to enabled the new zstd codec throughout the OIIO tool chain... suggestions welcome.

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.

Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
@TodicaIonut TodicaIonut marked this pull request as draft August 8, 2025 07:04
Copy link
Contributor

@jessey-git jessey-git left a comment

Choose a reason for hiding this comment

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

Beyond adding this to the documentation, adding a test would be good. See testsuite/openexr-compression/ for someplace that's easy to extend with zstd as a first step.

[edit] Though I don't immediately know a good pattern off-hand to use in the tests for when we need to restrict the test to only certain versions of openexr. Maybe someone else will chime in before I investigate that further tomorrow.

// that 5 is a great tradeoff between size and speed, so that is our
// default.
if (Strutil::istarts_with(comp, "zstd")) {
header.zstdCompressionLevel() = (qual >= 1 && qual <= 22) ? qual : 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be important to document these values, and zstd in general, in the src/doc/builtinplugins.rst file under the OpenEXR section.

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 for the patch.

Would you prefer to add documentation for zstd to builtinplugins.rst as part of this PR before I merge it? Or follow up with a second PR (right away, please), or do you want me to add that myself?

@lgritz
Copy link
Collaborator

lgritz commented Aug 8, 2025

Jesse makes a good point about the testing. I'm also not 100% sure how to best test openexr features that are only in some versions. And now that you mention it, I'm not sure we comprehensively test all the existing compression methods. This isn't necessarily making the situation any worse in practice, so I don't insist on fully solving that before merging this PR. But it would be good to shore up the testing in this regard. Also, and maybe this is longer term, but we conspicuously lack a way for an app to ask through the API what compression methods are supported by any given format.

Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
@TodicaIonut
Copy link
Contributor Author

openexr's features Zstd branch ZstdCorrectTypesize failing?

@lgritz
Copy link
Collaborator

lgritz commented Aug 8, 2025

Oh, wait, I just noticed that this PR is just a draft, and also that zstd support has not yet been merged into OpenEXR.

@pleprince
Copy link

pleprince commented Aug 8, 2025 via email

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.

This isn't a real change request, I'm just removing approval so I don't accidentally merge it without remembering that it's still a draft and that it assumes a feature not yet in OpenEXR. The code still seems fine for what it does.

@lgritz
Copy link
Collaborator

lgritz commented Aug 23, 2025

What should we do about this PR? The thing it's addressing isn't a feature in OpenEXR yet.

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