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

fix(openexr): modernize dwa compression level setting #4434

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 18, 2024

Starting with OpenEXR 3.1.3, the preferred API call for setting DWAA/DWAB compression level has changed. We never changed the OIIO side. Luckily, OpenEXR seems to have kept respecting the old API calls we were making (passing as an attribute). But this modernizes the approach, now that we don't have OpenEXR 2.x support to maintain.

But in the process, we also fixed a bug! Turns out we weren't
propagating the compression properly -- if the compression name
was plain "dwaa", we ignored any "openexr:dwaCompressionLevel"
attribute, effectively.

@lgritz lgritz force-pushed the lg-openexrcomp branch 3 times, most recently from 6b3b206 to 460b163 Compare September 20, 2024 23:58
Starting with OpenEXR 3.1.3, the preferred API call for setting
DWAA/DWAB compression level has changed. We never changed the OIIO
side. Luckily, OpenEXR seems to have kept respecting the old API calls
we were making (passing as an attribute). But this modernizes the
approach, now that we don't have OpenEXR 2.x suppor to maintain.

But in the process, we also fixed a bug! Turns out we weren't
propagating the compression properly -- if the compression name
was plain "dwaa", we ignored any "openexr:dwaCompressionLevel"
attribute, effectively.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Sep 20, 2024

@meimchu does this look sensible to you?

@meimchu
Copy link
Contributor

meimchu commented Sep 21, 2024

@meimchu does this look sensible to you?

I dug up some information just simply to provide more info for you to consider if necessary:

https://docs.thinkboxsoftware.com/products/deadline/10.3/1_User%20Manual/manual/app-redline.html
https://docs.red.com/955-0004/REDCINE-XProOperationGuide/Content/C_RedlineArgument/OpenEXR.htm
Both indicate that there is a range for the DWA compression value to be between 25 and 100? Seems wrong because I was able to crank it to 500 in one of my tests. I just thought it'd be a good idea to figure out what is the range limit so you can account for that.

https://openexr.com/en/latest/ReadingAndWritingImageFiles.html
Prior to OpenEXR 3.1.3, the default value for zip compression level is 6. Then it got changed to 4.

I don't know if it matters, but I guess you are getting an integer from CompressionQuality metadata. Then converting it to float. I guess that means oiiotool will not handle cases requiring decimal precision? Hopefully that won't be a thing!

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.

2 participants