-
Notifications
You must be signed in to change notification settings - Fork 11
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
added scalar overflow handling #57
Conversation
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.
I think that use_16bit_dc_estimate param to the DecompressWrapper function should become use_scalar_lepton and to control both flags - use_16bit_dc_estimate and use_16bit_adv_predict. Also, is there are a reason to keep these flags separated in EnabledFeatures?
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.
Thanks. I missed that this change is not needed for back-compat, but it improves the compression for future compressed files and contains a small format change to support it. I have added a new comment, I think there is a small bug here.
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.
LGTM, except for the non-existing documentation in the README.
Edit: and sorry for the delay! :/
Thanks for fixing! |
@@ -159,14 +159,19 @@ pub unsafe extern "C" fn WrapperDecompressImageEx( | |||
) -> i32 { |
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.
In the comment above please update the description to "that were compressed by C++ SIMD version of Lepton"
One other place where there was a difference between the 32 bit scalar and 16 bit SIMD code. Unfortunately this is the opposite behavior of what was there before, so for backward compat I had to add another feature setting for that.