Add API for fetching skippable frame content #2708
Conversation
binhdvo
force-pushed
the
skippable
branch
4 times, most recently
from
eb4ec16
to
969f660
Compare
lib/zstd.h
Outdated
| * | ||
| * @return : number of bytes written or a ZSTD error. | ||
| */ | ||
| ZSTDLIB_API unsigned long long ZSTD_readSkippableFrame(void* dst, size_t dstCapacity, unsigned* magicVariant, |
There was a problem hiding this comment.
nit: A ZSTD error sum type (error | size) cannot be an unsigned long long. This will have to return a size_t.
It is fine to return a size_t because we know the data is contiguous, so a size_t is large enough.
There was a problem hiding this comment.
I think this caused one of the CI tests to fail before, let me try again
binhdvo
force-pushed
the
skippable
branch
7 times, most recently
from
277dc93
to
57edb43
Compare
lib/decompress/zstd_decompress.c
Outdated
| if (zfh.frameType == ZSTD_skippableFrame) { | ||
| return readSkippableFrameSize(src, srcSize); | ||
| } else { | ||
| return ZSTD_findFrameCompressedSize(src, srcSize); |
There was a problem hiding this comment.
Isn't ZSTD_findFrameCompressedSize() already able to provide the size of a skippable frame ?
There was a problem hiding this comment.
Ah, I thought that was only for compressed frames; removed the new function.
| /* deliver payload */ | ||
| if (skippableContentSize > 0 && dst != NULL) | ||
| ZSTD_memcpy(dst, (const BYTE *)src + ZSTD_SKIPPABLEHEADERSIZE, skippableContentSize); | ||
| if (magicVariant != NULL) |
There was a problem hiding this comment.
Does the API allow magicVariant == NULL ?
In which case, the documentation should mention it and tell what happens in this case.
If not, an assert() here would be more appropriate.
Finally, an alternative design could be to return a struct that combine the 2 results (size_t bytes written, and unsigned magic variant), making them clearly return values, and avoiding potential confusion with regards to caller-side allocation, in+out values, etc.
|
Almost there ! |
lib/zstd.h
Outdated
| * in the magicVariant. | ||
| * | ||
| * Returns an error if destination buffer is not large enough, if the source size is not representable | ||
| * with a 4-byte unsigned int, or if the frame is not skippable. |
There was a problem hiding this comment.
minor comment :
if the source size is not representable * with a 4-byte unsigned int
I think the binary format does not allow such scenario. Skippable frame content size must be represented as a 4-bytes unsigned int.
Provides functions for navigating frames in memory and identifying and extracting content from skippable frames. Streaming mode left out of scope for now.