-
Notifications
You must be signed in to change notification settings - Fork 656
Add gdeflate compression and decompression with NVIDIA fork of libdeflate. #2153
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
base: main
Are you sure you want to change the base?
Conversation
…late. - Gdeflate compression can yield multiple 64K pages. - Each chunk includes number of pages and size of each page. - Off by default. gdeflate calls are wrapped and conditionally compiled in. - CMake and Bazel builds can optionally fetch NVIDIA fork of libdeflate.
|
|
Thanks for this! I'll look into the failing CI checks this weekend, I think I know what's going on. The project policy is to require a signed Contributor License Agreement. I don't recall if Nvidia has an agreement in place, but is that something you can follow up on? Also, we require signed git commits. Could you amend your commit with "-s"? That adds the "Signed-off-by" line. Note it's not sufficient to submit an additional commit, they all have to be signed, so you'll need to amend the commit and force-push. Or close this PR and submit a fresh one. |
| message(STATUS "Using externally provided libdeflate: ${EXR_DEFLATE_VERSION}") | ||
| # For OpenEXR.pc.in for static build | ||
| set(EXR_DEFLATE_PKGCONFIG_REQUIRES "libdeflate >= ${EXR_DEFLATE_VERSION}") | ||
| elseif(OPENEXR_FORCE_FETCHCONTENT_DEFLATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOT OPENEXR_FORCE_INTERNAL_DEFLATE AND OPENEXR_FORCE_FETCHCONTENT_DEFLATE?
Otherwise the check for gdeflate support below doesn't work if both are somehow set to ON. Or have a mechanism (throw an error?) to ensure they're exclusive?
| ) | ||
|
|
||
| # Toggle this to fetch the NVIDIA libdeflate fork, then update OPENEXR_ENABLE_GDEFLATE in BUILD.bazel. | ||
| _enable_gdeflate = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could have a flag - like in https://github.com/bazelbuild/bazel-central-registry/blob/0040dfca0c14b4eda9e079fb51fde2567c39a8d9/modules/boost.asio/1.89.0/overlay/BUILD.bazel#L5
In boost.asio one can decide to use OpenSSLvs. BoringSLL vs. No SSL support
Untested (inspiered by boost.asio):
string_flag(
name = "defalte",
build_setting_default = "libdeflate",
values = [
"libdefalte",
"ibdefalate_nvl",
],
visibility = ["//visibility:public"],
)
config_setting(
name = "libdeflate",
flag_values = {":defalte": "libdeflate"},
)
config_setting(
name = "libdeflate_nv",
flag_values = {":defalte": "libdeflate_nv"},
)
...
deps = [
...
] + select({
":libdeflate": ["@libdeflate//:deflate"], # or maybe use default attirbute here
":libdeflate_nv": ["@libdeflate_nv//:deflate"]
}),
Someone who uses OpenEXR (or your CI job) can than add the flag
build --@openexr//:deflate=libdeflate_nv (this could also be added to the .bazelrc)
|
@MarkLeone, looking at this a little deeper, I think the better configuration is to simply vendor in the source code directly from NVIDIA/libdeflate, and avoid the FetchContent altogether. Look in share/util/check_deflate.sh, which is a helper script to be run manually whenever a new version of libdeflate is released. In essence, that's what you've done, but with a forked repo. I think if you change the URL in that script to src/lib/OpenEXRCore/compression.c includes only the files out of libdeflate that are needed, the rest are ignored. You'll need to add whatever gdeflate-related files are needed there. |
|
@MarkLeone, it looks like NVIDIA/libdeflate is based on a very old fork of ebiggers/libdeflate (v1.8 from 2021), which was before libdeflate introduced cmake support, which complicates the setup, since your library has no cmake config. I started working on fixing the CI to test against an external build of NVIDIA/libdeflate, but that's going to require some extra hoops to jump through. |
Additional info and discussion on openexr-dev@lists.aswf.io