feat: add zstd compression #451
Conversation
Rdataflow
force-pushed
the
feat--add_zstd
branch
2 times, most recently
from
516c9b9 to
f80f727
Compare
Rdataflow
force-pushed
the
feat--add_zstd
branch
from
f80f727 to
d497708
Compare
|
|
||
| export default class ZstdDecoder extends BaseDecoder { | ||
| decodeBlock(buffer) { | ||
| return zstd.decode(new Uint8Array(buffer), 1000_000_000).buffer; |
There was a problem hiding this comment.
The second parameter is uncompressedSize, which is fixed here to 1 million. Is this significant? can we actually know it beforehand? What if the size is actually smaller/larger?
There was a problem hiding this comment.
Reading the source: if uninitialized (i.e: 0) the size should automatically be found out. Wouldn't that be a better solution?
I'm worried, that if the actual size is larger than the 1 million bytes buffer (which can easily be reached by 1024x1024 tile sizes) we might run into troubles.
There was a problem hiding this comment.
@constantinius this value of 1 billion has been approximated using trial and error.
AFAICS 0 or omitting the param fails in the CI tests for some unknown reason.
however huge numbers make it pass w/o issues.
I couldn't observe any negative impact neither with 100 billion. (i.e. no mem limits hit)
for the full journey see this branch https://github.com/Rdataflow/geotiff.js/commits/ci-test/ and it's CI tests failing or passing
that's what drove us here... but maybe there exist even better ideas?
There was a problem hiding this comment.
Hey @Rdataflow
I investigated this. There is a function in zstd to determine the uncompressed size of a chunk. This is a property set in the chunk itself which is optional. When it is optional, zstd says to read the chunk in streaming mode. Otherwise proper decompression is not guaranteed. And this is something I'd like to avoid, since it may work on our test files, but not on random files used by people. So I'm against abusing the zstd library that way. Unfortunately, the zstd library we are currently using (and no other one I investigated) supports streaming decompression as it does not wrap the necessary functions of the wasm library.
I'm now trying to implement this streaming decoding into the zstd library. I'm thinking of incorporating that into the source code of geotiff.js directly, as it may be easier to handle.
There was a problem hiding this comment.
Hey @constantinius
Great you'll an even better way to implement zstd decompression 😃
just in case that becomes too heavy, I'll share my thoughts on alternative ways (up to you to decide and implement properly):
- precalculate the uncompressed buffer size using the geotiff properties (tile size, bit depth, etc.)
@constantinius this PR brings ZSTD support 😄
thanks for your review ...
Closes: #372