Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow negative zstd compression level #726

Closed
wants to merge 2 commits into from
Closed

Conversation

ahupna
Copy link

@ahupna ahupna commented Aug 21, 2023

It is not documented properly: facebook/zstd#3133
But zstd compression level can go negative, and larger than int16_t as well: https://news.ycombinator.com/item?id=31428760

Changed the preset type in the mz_stream_zstd to be a signed int32_t and removed < 0 clamping when setting level.

@nmoinvaz
Copy link
Member

I am currently using MZ_COMPRESS_LEVEL_DEFAULT (-1), so how does this work with that? Does that value need to be changed to something else?

@ahupna
Copy link
Author

ahupna commented Aug 23, 2023

The default level from zstd.h is 3:
#ifndef ZSTD_CLEVEL_DEFAULT
# define ZSTD_CLEVEL_DEFAULT 3
#endif
If possible i would try to apply the actual zstd default, not -1(now) or 6(before).

I've pushed a change in mz_strm_zstd.c where the ZSTD_CLEVEL_DEFAULT is used when creating the stream, this is now similar to bzip/lzma and zlib.

Do you want me to try and change the default level in minizip.c when parsing arguments (that is what you are talking about right?)
I'm not familiar with project but was thinking you could assign the default compression level after parsing the arguments where you know if ZSTD is selected and if the user set the compression level, or not.
If not, you could reassign it for the ZSTD method to 3.

@mkasick
Copy link
Contributor

mkasick commented Sep 12, 2023

I looked at this while working on #730, and I'm not sure the complexity of shoehorning support for negative levels as part of minizip's command-line options is worthwhile. The use case for negative levels is to support compression of high-bandwidth streaming data, which isn't within scope of an archive file format. I think support for zstd's "regular" levels of 1 through 22 is sufficient.

@nmoinvaz nmoinvaz added the zstd Facebook zstd compression library label Oct 19, 2023
nmoinvaz added a commit that referenced this pull request Oct 26, 2023
@nmoinvaz
Copy link
Member

I have made some commits that allow for negative values (just not -1), through the API. It is still not available through the command line. If somebody wants to tackle that, I am open to it, but having the double dashes for command line args is a bit weird when it is normally only single dash.

@nmoinvaz nmoinvaz closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zstd Facebook zstd compression library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants