Libdeflate port #25
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| #include <gzip/config.hpp> | ||
|
|
||
| // zlib | ||
| #include <libdeflate.h> | ||
|
|
||
| // std | ||
| #include <limits> | ||
|
|
@@ -12,85 +12,74 @@ namespace gzip { | |
|
|
||
| class Decompressor | ||
| { | ||
| std::size_t const max_; | ||
| struct libdeflate_decompressor* decompressor_ = nullptr; | ||
artemp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // make noncopyable | ||
| Decompressor(Decompressor const&) = delete; | ||
| Decompressor& operator=(Decompressor const&) = delete; | ||
|
|
||
| public: | ||
| Decompressor(std::size_t max_bytes = 2147483648u) // by default refuse operation if required uutput buffer is > 2GB | ||
|
There was a problem hiding this comment. @artemp typo: |
||
| : max_(max_bytes) | ||
| { | ||
| decompressor_ = libdeflate_alloc_decompressor(); | ||
| if (!decompressor_) | ||
| { | ||
| throw std::runtime_error("libdeflate_alloc_decompressor failed"); | ||
| } | ||
| } | ||
|
|
||
| ~Decompressor() | ||
| { | ||
| if (decompressor_) | ||
| { | ||
| libdeflate_free_decompressor(decompressor_); | ||
| } | ||
| } | ||
|
|
||
| template <typename OutputType> | ||
| void decompress(OutputType& output, | ||
| char const* data, | ||
| std::size_t size) const | ||
| { | ||
| // https://github.com/kaorimatz/libdeflate-ruby/blob/0e33da96cdaad3162f03ec924b25b2f4f2847538/ext/libdeflate/libdeflate_ext.c#L340 | ||
| // https://github.com/ebiggers/libdeflate/commit/5a9d25a8922e2d74618fba96e56db4fe145510f4 | ||
artemp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| std::size_t actual_size; | ||
| std::size_t uncompressed_size_guess = std::min(size * 4, max_); | ||
|
There was a problem hiding this comment. What happens when |
||
| output.resize(uncompressed_size_guess); | ||
|
There was a problem hiding this comment. This probably needs to be re-written to avoid needing to guess the size up front. @artemp could you tackle that? There was a problem hiding this comment.
diff --git a/include/gzip/decompress.hpp b/include/gzip/decompress.hpp
index badd315..627e670 100644
--- a/include/gzip/decompress.hpp
+++ b/include/gzip/decompress.hpp
@@ -57,17 +57,23 @@ class Decompressor
// https://github.com/ebiggers/libdeflate/commit/5a9d25a8922e2d74618fba96e56db4fe145510f4
std::size_t actual_size;
std::size_t uncompressed_size_guess = size * 4;
- output.resize(uncompressed_size_guess);
- enum libdeflate_result result = libdeflate_gzip_decompress(decompressor_,
- data,
- size,
- static_cast<void*>(&output[0]),
- uncompressed_size_guess, &actual_size);
- if (result == LIBDEFLATE_INSUFFICIENT_SPACE)
+ output.reserve(uncompressed_size_guess);
+ enum libdeflate_result result;
+ for (;;)
{
- throw std::runtime_error("no space: did not succeed");
+ result = libdeflate_gzip_decompress(decompressor_,
+ data,
+ size,
+ const_cast<char*>(output.data()),
+ output.capacity(), &actual_size);
+ if (result != LIBDEFLATE_INSUFFICIENT_SPACE)
+ {
+ break;
+ }
+ output.reserve((output.capacity() << 1) - output.size());
}
- else if (result == LIBDEFLATE_SHORT_OUTPUT)
+
+ if (result == LIBDEFLATE_SHORT_OUTPUT)
{
throw std::runtime_error("short output: did not succeed");
}
Couple notes:
/cc @springmeyer There was a problem hiding this comment. @artemp great fix - can you please commit directly to this branch? But two concerns to keep in mind:
There was a problem hiding this comment.
Good point, I'll add conditional exception in the loop if allocation size is getting to big.
Let me investigate and fix this ^ I'm going to push updates and continue work on this branch There was a problem hiding this comment.
👍 I think you'll likely need an easy way to test larger, varied files and a CLI would allow that. So could you finish #29 to aid your testing of this branch? |
||
| enum libdeflate_result result; | ||
| for (;;) | ||
| { | ||
| result = libdeflate_gzip_decompress(decompressor_, | ||
| data, | ||
| size, | ||
| const_cast<char*>(output.data()), | ||
| output.size(), &actual_size); | ||
| if (result != LIBDEFLATE_INSUFFICIENT_SPACE) | ||
| { | ||
| break; | ||
| } | ||
| if (output.size() == max_) | ||
| { | ||
| throw std::runtime_error("request to resize output buffer can't exceed maximum limit"); | ||
| } | ||
| std::size_t new_size = std::min((output.capacity() << 1) - output.size(), max_); | ||
| output.resize(new_size); | ||
| } | ||
|
|
||
| if (result == LIBDEFLATE_SHORT_OUTPUT) | ||
| { | ||
| throw std::runtime_error("short output: did not succeed"); | ||
| } | ||
| else if (result == LIBDEFLATE_BAD_DATA) | ||
| { | ||
| throw std::runtime_error("bad data: did not succeed"); | ||
| } | ||
| else if (result != LIBDEFLATE_SUCCESS) | ||
| { | ||
| throw std::runtime_error("did not succeed"); | ||
| } | ||
| output.resize(actual_size); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that this approach (initialize C struct pointer in constructor + free the memory in the deconstructor) is applying RAII (https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization) to avoid a memory leak.