Skip to content

Zstd compression support - #2590

Open
cary-ilm wants to merge 7 commits into
mainfrom
zstd-beta
Open

Zstd compression support#2590
cary-ilm wants to merge 7 commits into
mainfrom
zstd-beta

Conversation

@cary-ilm

Copy link
Copy Markdown
Member

[repeat of #2586 from the main repo's zstd-beta, not from vlazar-ilm:zstd-beta]

This is the merge to main from: #2458
Some more details can be found here:
Proposal_ Support for Zstd Compression in OpenEXR.pdf

This PR adds a new compression method based on Zstd with some preprocessing. While the method was heavily tested with scanline Deep data, it supports both flat and tiled formats.
It introduces a new dependency on the Zstd library. I also added a vendored copy inside the "external" directory.

I have tested the method with scanline deep renders totalling about 2.5TB ( 45k files ), a large majority being deep alpha only. Over this corpus, using exrmetric, it achieved (size weighted):

By default Zstd compresses with level 5, very low effort ( valid range is between 1-22). Higher levels achieve better compression but for considerably low write speed - decompression speed is almost unaffected.

Weighted Size Ratio vs Zips: 0.671
Weighted Write Speed Ratio vs Zips: 0.661
Weighted Read Speed Ratio vs Zips: 0.771

For flat scanlines comparison with ZIPS:
Weighted Size ratio: 0.975
Write time ratio: 0.72
Weighted Read time ratio: 1.01

Compression pipeline description:

The incoming data is in a "row major" planar format: within a chunk, every row channels are contiguous. The first step is to convert it to a full planar format: a single channel is contiguous across all rows inside a chunk.
sort the data into 2 blocks: 2 bytes and 4 bytes wide. This makes the byteshuffle and delta easier
3.(optional, currently disabled) is a neighboring pixel delta encoding (performed in in the appropriate int arithmetic). This is currently disabled as it helps certain sequences but hurts others. I did an experiment where I recompress the scanline twice and pick the best, which yielded about 5% size improvement, but compression time was doubled (might get some of that time back by doing some smarter heuristic). Also the payoff and penalty was asymmetric (it hurts a lot more than it helps on certain images).
A byte shuffle is performed on each 2 and 4 byte block. This takes advantage of the fact that exponents and high order mantissa bits are often similar across pixels.
The two blocks are then compressed with Zstd.
1-2 are performed in 2 step with a computed lookup table.

The Shuffle and delta were implemented with scalar code for compression but for decompression with AVX2 intrinsics for GCC (mostly as a proof of correctness).

A few quirks:

For this method, I need the sample count table. I was unable to get access to it inside the compressor. When using the C++ API, the data was constantly wiped so I added a new version of the C++ compressor functions that passes this point and added guards inside EXRCore to protect it from getting wiped.
The C++ Deep Tiled path was calling the wrong compressor function that was causing issues with my codec.
Fixed an overflow issue with exrmetric when passing very large files.
The Zstd context is saved as a TLS that gets dellocated only when a thread dies.
Possible areas of improvement (all do not require invalidating the current implementation):

Add a fastpath to skip channel sorting if channels are already grouped , the sort is NOOP
Add a multi scanline variant: in some tests i got about 5% more with 16 scanlines
Add cheap heuristic and enable delta compression for certain scanlines
Tweak the ZStd compression settings: Zstd has a few settings that could make it generate slightly smaller files with higher compression speed

vlazar-ilm and others added 6 commits August 12, 2026 12:39
Signed-off-by: Vlad Lazar <vlazar@ilm.com>
(cherry picked from commit 3223e18)

Signed-off-by: Vlad Lazar <vlazar@ilm.com>
Signed-off-by: Todica Ionut <todicaionut2000111@gmail.com>
* Update doc about the new dependencies

Signed-off-by: Vlad Lazar <vlazar@ilm.com>

* Document the zstd compression level

Signed-off-by: Vlad Lazar <vlazar@ilm.com>

* Apply suggestion from @cary-ilm

next release is v3.5

Signed-off-by: Cary Phillips <seabeepea@gmail.com>

* Apply suggestion from @cary-ilm

Signed-off-by: Cary Phillips <seabeepea@gmail.com>

* Apply suggestion from @cary-ilm

Signed-off-by: Cary Phillips <seabeepea@gmail.com>

---------

Signed-off-by: Vlad Lazar <vlazar@ilm.com>
Signed-off-by: Cary Phillips <seabeepea@gmail.com>
Co-authored-by: Cary Phillips <seabeepea@gmail.com>
The new EXR-ZSTD decode path (internal_exr_undo_zstd /
exr_undo_zstd_v1) sized its scratch/decompression buffer directly
from ZSTD_getFrameContentSize(), a value read out of the untrusted
compressed chunk payload itself. A hand-crafted zstd frame can
declare an arbitrary content size there while its real body decodes
to something much smaller, letting a malicious EXR file drive the
destination buffer capacity (and thus the capacity handed to
ZSTD_decompressDCtx) far beyond what the chunk's already-validated
uncompressed_size allows.

Combined with this, ensure_tls_resources() computed its 25% buffer
overshoot (required_size + (required_size >> 2)) and 64-byte
alignment step with no overflow checking, and did not reset its
cached buffer/size on allocation failure. With a sufficiently large
untrusted required_size this arithmetic wraps around, so a
small/tiny buffer would be allocated while the code believed it was
large enough, and the same undersized-but-successfully-allocated
buffer could be reused with a stale, incorrect belief of its size on
a later call.

Fixes:
- Bound the ZSTD frame's declared content size to at most
  uncompressed_size + 16 (the maximum possible pre-ZSTD inner-stream
  overhead: up to two 8-byte segment length prefixes), rather than
  trusting it unconditionally.
- Make ensure_tls_resources() overflow-safe: check for wraparound in
  both the 25% overshoot and the 64-byte alignment step, return a
  success/failure status, and reset shuffle_buf/shuffle_buf_size to
  a known-empty state on any failure so a later, smaller request
  cannot mistake a stale or invalid buffer for a valid one.
- Update both call sites (encode and decode) to check the new return
  value and fail cleanly instead of proceeding with a NULL or
  undersized buffer.
- Add a guard against unsigned underflow in the comp_buf_size -
  ZSTD_EXR_V1_HEADER computation in exr_undo_zstd_v1(), in case the
  caller's existing size guarantee is ever relaxed by future
  refactoring.

Assisted-by: GitHub Copilot CLI (claude-sonnet-5)

Signed-off-by: Cary Phillips <seabeepea@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@cary-ilm

Copy link
Copy Markdown
Member Author

CodeQL alert review: cpp/suspicious-add-sizeof (alert #150)

This appears to be a false positive, not a vulnerability introduced by this PR.

The flagged line, src/lib/OpenEXRCore/internal_structs.c:328:

ret->user_data = (((uint8_t*) memptr) + sizeof (struct _priv_exr_context_t));

is pre-existing, unchanged code (identical on main, just at an earlier line number). This PR only adds new lines earlier in internal_exr_alloc_context() (for default_zstd_level/zstd_compression_level), which shifted this line into the PR's diff hunk. CodeQL's "new alerts in code changed by this PR" surfaces any alert whose line falls within a changed diff region, not just lines actually modified by the PR — a known heuristic limitation, not a regression from this change.

The code itself is correct: memptr is cast to uint8_t* (a 1-byte-wide pointer type), so + sizeof(struct _priv_exr_context_t) adds that many bytes, as intended, to skip past the header struct to the trailing extra-data region. The rule targets double-scaled offsets like intPtr + n * sizeof(int); that pattern isn't present here.

Recommend dismissing alert #150 as a false positive (or filing a separate, unrelated cleanup if a suppression annotation is desired) rather than blocking this PR on it.

Propogate the found/vendored version of zstd into internal macros,
following the pattern of OpenJPH and libdeflate.

This also formats the informational messages to be consistent with
openjph and Imath.

Assisted-by: Copilot GPT-5.6 Sol

Signed-off-by: Cary Phillips <seabeepea@gmail.com>
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.

3 participants