Add encoder selector to LZ4 plugin 32004 via cd_values[1]#267
Conversation
A new signed `cd_values[1]` slot selects LZ4HC level (positive), fast-encoder acceleration (negative), or the legacy LZ4_compress_default (zero/absent), with HC level clamped to [`LZ4HC_CLEVEL_MIN`, `LZ4HC_CLEVEL_MAX`]. The on-disk chunk format is unchanged, so existing archives and the reverse filter continue to work without modification.
ajelenak
requested review from
bmribler,
brtnfld,
fortnern,
glennsong09,
jhendersonHDF,
lrknox and
mattjala
as code owners
| static int | ||
| lz4_encode(const char *src, char *dst, int srcSz, int dstCap, int encoderParam) | ||
| { | ||
| if (encoderParam > 0) { |
There was a problem hiding this comment.
| if (encoderParam > 0) { | |
| if (encodeParam < LZ4_CLEVEL_MIN) encodeParam = LZ4_CLEVEL_MIN; | |
| if (encodeParam > LZ4_CLEVEL_MAX) encodeParam = LZ4_CLEVEL_MAX; | |
| if (encoderParam >= LZ4HC_CLEVEL_MIN) { |
long isn't always wider than int so I think it is safer to clamp encoderParam to its usable range at the start before doing any math.
Using >= LZ4HC_CLEVEL_MIN matches the behavior in https://github.com/lz4/lz4/blob/6cf42afbea04c9ea6a704523aead273715001330/lib/lz4frame.c#L956
There was a problem hiding this comment.
LZ4_CLEVEL_MAX is LZ4HC_CLEVEL_MAX and LZ4_CLEVEL_MIN is -(LZ4_ACCELERATION_MAX - 1)
There was a problem hiding this comment.
Agreed — long is 32-bit on LLP64, so the widen-before-negate didn't actually protect INT_MIN. Now clamped to the usable range before any arithmetic.
There was a problem hiding this comment.
LZ4_CLEVEL_MAXisLZ4HC_CLEVEL_MAXandLZ4_CLEVEL_MINis-(LZ4_ACCELERATION_MAX - 1)
Those aren't exposed as macros in liblz4, so I used LZ4HC_CLEVEL_MAX for the upper bound and a locally-mirrored, #ifndef-guarded LZ4_ACCELERATION_MAX for the lower bound.
There was a problem hiding this comment.
Looks good, but I think we should try to match how compression levels are used in the lz4frame library.
Here is my understanding of how it works there.
-65536 to -1 are mapped to fast mode with acceleration 65537 to 2
0 and 1 are default fast mode.
2 to 12 are LZ4HC levels.
Everything outside -65536 to 12 is clamped to that range.
| } | ||
| else if (encoderParam < 0) { | ||
| /* Widen before negating so encoderParam == INT_MIN cannot overflow. */ | ||
| long accel = -(long)encoderParam; |
There was a problem hiding this comment.
int const acceleration = (level < 0) ? -level + 1 : 1;
Is the standard conversion from lz4frame: https://github.com/lz4/lz4/blob/6cf42afbea04c9ea6a704523aead273715001330/lib/lz4frame.c#L913
| level = LZ4HC_CLEVEL_MAX; | ||
| return LZ4_compress_HC(src, dst, srcSz, dstCap, level); | ||
| } | ||
| else if (encoderParam < 0) { |
There was a problem hiding this comment.
| else if (encoderParam < 0) { | |
| else { |
| * LZ4_ACCELERATION_MAX (currently 65537; see lz4.c). */ | ||
| return LZ4_compress_fast(src, dst, srcSz, dstCap, (int)accel); | ||
| } | ||
| return LZ4_compress_default(src, dst, srcSz, dstCap); |
There was a problem hiding this comment.
| return LZ4_compress_default(src, dst, srcSz, dstCap); |
There was a problem hiding this comment.
Both done — the fast branch now handles 0/1 at acceleration 1, which is exactly LZ4_compress_default.
nhz2
left a comment
nhz2
left a comment
There was a problem hiding this comment.
Looks good. Added a few minor comments and a suggestion for improved error handling.
| blockSize = nbytes; | ||
| } | ||
| nBlocks = (nbytes - 1) / blockSize + 1; | ||
| maxDestSize = nBlocks * LZ4_compressBound(blockSize) + 4 + 8 + nBlocks * 4; |
There was a problem hiding this comment.
Before the call to malloc we should check that LZ4_compressBound(blockSize) hasn't overflowed.
This also protects the (int)blockSize casts later on.
Something like:
| if (blockSize > (size_t)LZ4_MAX_INPUT_SIZE) { | |
| goto error; | |
| } | |
| maxDestSize = nBlocks * LZ4_compressBound(blockSize) + 4 + 8 + nBlocks * 4; |
|
|
||
| ## Encoder choice | ||
|
|
||
| The on-disk format above is independent of the encoder the writer used. | ||
| The plugin's `cd_values[1]` selects between `LZ4_compress_default` | ||
| (value `0`, the default), `LZ4_compress_HC` (positive value, HC level), | ||
| and accelerated `LZ4_compress_fast` (negative value, acceleration = | ||
| `-cd_values[1]`). All three produce LZ4 Block payloads that the same | ||
| `LZ4_decompress_safe` path reads, so files written with any encoder | ||
| choice are bitstream-compatible with readers that only know the default. |
There was a problem hiding this comment.
This is redundant with the information in the RegisteredFilterPlugins.md so can be removed to avoid it getting outdated. This document is to describe the on disk format, so the compression level details are not need here.
3 participants
A new signed
cd_values[1]slot selects LZ4HC level (positive), fast-encoder acceleration (negative), or the legacy LZ4_compress_default (zero/absent), with HC level clamped to [LZ4HC_CLEVEL_MIN,LZ4HC_CLEVEL_MAX]. The on-disk chunk format is unchanged, so existing archives and the reverse filter continue to work without modification.Closes #244.