Add gdeflate compression and decompression with NVIDIA fork of libdeflate.#2153
Add gdeflate compression and decompression with NVIDIA fork of libdeflate.#2153MarkLeone wants to merge 1 commit into
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.
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.
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