Switch to embedding libdeflate into EXRCore #1387
Conversation
docs/TechnicalIntroduction.rst
Outdated
| decompression, but ZIP compression is significantly slower. Photographic | ||
| images tend to shrink to between 45 and 55 percent of their uncompressed | ||
| size. | ||
| open source defulate / zlib library. ZIP decompression is faster than PIZ |
|
Do you want to entertain a discussion of the merits of FetchContent, which needs internet access at "cmake configure" time, versus git submodules, which need internet access only at clone time, versus at least supporting the option to get libdeflate from a previously built local or system install, from the point of view of those users who may build with very restricted internet access? |
|
I'm extremely excited for this one, thanks for taking it on! |
absolutely open to the discussion, I just control with the option and point it to a pre-cloned repo (OPENEXR_DEFLATE_REPO / TAG variables) usually, but submodule would work great as well, and have the bonus of not cloning into the build folder if you do a make clean (I would like to change the images to do the same thing that seem to download every time I wipe the build folder if that is what we want to do)... |
| images tend to shrink to between 45 and 55 percent of their uncompressed | ||
| size. | ||
| open source deflate library for zlib compression. ZIP | ||
| decompression is faster than PIZ decompression, but ZIP may be |
There was a problem hiding this comment.
It seems that ‘compression’ was deleted after ‘ZIP’ and needs to be re-added.
There was a problem hiding this comment.
how so? I re-wrapped the paragraph, but all the words are there...
|
@kdt3rd I don't have a strong opinion on the right way to embed. I always wonder myself, am indecisive enough that I usually just punt and assume it's already installed somewhere (or maybe write a short install_blah.sh). So I think you should interpret my comment as "Is that the right way to do it? If so, I should do it in other projects, too. But if you didn't have a clear methodology in mind when you wrote it, make sure you are considering the different permutations of internet availability that people may be subject to." |
|
If necessary we can tweak the methodology to allow external libdeflate sources to be provided if it turns out to be an issue. This is how we deal with it in OpenTimelineIO ~ if you override a variable pointing to rapidjson, we use it, otherwise we FetchContent into __deps. Same for our other dependencies. This meets my needs which are to not introduce a zlib dependency when using only OpenEXRCore, to use deflate for performance, and accomodate the full interring of deflate so that it's symbols are invisible to the thing linking OpenEXRCore. |
Need to do an experiment of what happens when there's a submodule and you package up a release tar.gz - that might be the deciding factor to me, if that pulls in and includes in the tarball or not. I was just following what was already in the setup for zlib / Imath (other than not actually recursing and compiling it). It would arguably be simpler to have a submodule, other than having to teach people to do git submodule --init --update ... Although would it be weird to do submodule for one (deflate) and fetchcontent for Imath? I think fetchcontent is the right thing to do for Imath, given people might want to be poking around in that source. Does that add to the cognitive load, or simplify it? |
BUILD.bazel
Outdated
| @@ -141,6 +141,144 @@ cc_library( | |||
| deps = [":Iex"], | |||
| ) | |||
|
|
|||
| _DEFLATE_SRCS = [ | |||
There was a problem hiding this comment.
This should be moved to libdeflate.BUILD - see my other comment
bazel/third_party/openexr_deps.bzl
Outdated
| "https://mirror.bazel.build/zlib.net/zlib-1.2.13.tar.gz", | ||
| "https://zlib.net/zlib-1.2.13.tar.gz", | ||
| ], | ||
| http_archive( |
There was a problem hiding this comment.
use maybe again:
maybe(
http_archive,
name = "libdeflate",
build_file = "@com_openexr//:bazel/third_party/libdeflate.BUILD",
sha256 = "225d982bcaf553221c76726358d2ea139bb34913180b20823c782cede060affd",
strip_prefix = "libdeflate-1.18",
urls = ["https://github.com/ebiggers/libdeflate/archive/refs/tags/v1.18.tar.gz"],
)
The whole point of maybe is that it checks if a dependency with the name libdeflate was already added - and if so it skips it - can happen in large project that use the same transitive dependencies
There was a problem hiding this comment.
yeah, I debated removing the maybe, but not being a bazelite, I didn't know if maybe gave it a chance to be overridden by the user (much like is possible in cmake where I can specify the (potentially local) repo...
There was a problem hiding this comment.
i think the maybe is now consistent with the others. Where I was embedding before, there wasn't a choice of maybe given the hijinks, now that it is a normal library, it's fine
|
Submoduling is problematic when a project itself is one that is likely to be submoduled, which is the case for OpenEXR. If my project, foo, submodules both libdeflate, and OpenEXR, you've got a problem. When people fetch foo, I've instructed them to do so recursively. That recursiveness is transitive to OpenEXR, so I'll end up with two libdeflates, possibly at different commits. libdeflate itself is a "leaf" but the problem frequently arises that somewhat large dependencies are roped in by submodules, such as glfw, various test frameworks, and so on. It's yet another variant of a dependency hell. Sticking to FetchContent, in the case that libdeflate was not otherwise detected, quarantines the libdeflate artifact and source trees to the build directory under __deps/libdeflate and never infects the source tree. I would only ever recommend submodules when the project in question is "outermost" and unlikely to be itself submoduled. |
Yes, please! The first thing we do as distro packagers is unbundle stuff like this and use shared system libraries. Edit: libjxl perhaps has the right balance of having these as submodules (optional through scripts), or using system libraries. |
|
This looks great! Last time I checked specifically on EXR files, libdeflate was significantly faster than zlib (https://aras-p.info/blog/2021/08/09/EXR-libdeflate-is-great/). |
this you can do with specifying the cache variable OPENEXR_LIBDEFLATE_REPO in the cmake chunklet in OpenEXRSetup.cmake (or the branch variable) to override what is used.... |
@aras-p what I didn't know is if in switching to libdeflate if I should switch the default compression level from a perf / size tradeoff standpoint - did you do any tests with that when you were looking at the zlib compression level? Either from your suggested switch from 6 -> 4, or from the blog post you've written? |
For the source, yes. But I cannot unbundle this change and choose a system library instead, like e.g. libjxl lets me do for most of its dependencies. I have 3 options there:
Most, if not all, packagers wanting to distribute OpenEXR will choose 2b: Arch: https://github.com/archlinux/svntogit-community/blob/20bd0349291a2fb11d49f6326804aef63a8599c4/trunk/PKGBUILD#L59-L62 etc. Basically, this could be handled identically to Imath (w/ added complexity of local CMake find module since not all libdeflate versions ship CMake config files). |
@kdt3rd in my tests level 4 with libdeflate made the most sense too, just like with zlib. There's a tiny loss of comp. ratio, but writing the files is much faster at level 4 compared to level 6. |
Switches to embedding upstream (MIT license) libdeflate vs linking against external library, functions should be private / hidden by default. Like Imath, enables one to use a system version or force using an internally embedded version (OPENEXR_FORCE_INTERNAL_DEFLATE). Further, the repository used can be controlled with OPENEXR_DEFLATE_REPO and OPENEXR_DEFLATE_TAG cmake variables. This centralizes all IETF RFC 1950 (zlib) compression into the EXRCore library. This implies that the main C++ library depends on EXRCore. Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>
kdt3rd
force-pushed
the
switch_to_deflate
branch
from
bbaaaae
to
bd76117
Compare
I have switched to enabling the system version to be used if available, or embedding if not. Further, fixed such that symbols should be hidden if openexr is then used within another project that also uses deflate somehow. |
Great, thanks! |
Switches to embedding upstream (MIT license) libdeflate vs linking against external library, functions should be private / hidden by default
use EXRCore library in main C++ library