This one is a bug rather than an improvement, and is independent of the two log2 replacements I have raised separately in #5.
let bits_per_residual =
(error * error_scale).ln() / (2.0 * std::f64::consts::LN_2).max(0.0);
.max(0.0) binds to (2.0 * std::f64::consts::LN_2), which is a positive constant — so the clamp is a no-op and the expression is just ln(error * error_scale) / (2 * ln 2). Almost certainly the intent was to clamp the quotient:
let bits_per_residual =
((error * error_scale).ln() / (2.0 * std::f64::consts::LN_2)).max(0.0);
It matters because error * error_scale < 1 makes the logarithm negative, and bits_per_residual with it — and that value is multiplied by the residual count in subframe_bits on the next line. So a subframe can be costed at a negative bit count during order selection, and the order search prefers whichever order drives the residual error lowest rather than the one that actually codes smallest.
We applied this fix and measured it. Three of our test fixtures each came out 43 bytes smaller — about 0.3 % better compression on ~13.8 KB files — because order selection stops preferring subframes it was costing at a negative bit count. The effect is in the direction the analysis predicts.
Two caveats worth stating plainly:
- Three fixtures is not a corpus. Correct costing should be better on average, but we have not proven it is better on every input, and you would want to run your own material before taking it.
- It changes the encoded bitstream wherever the branch is reachable. But FLAC is lossless, so this is a smaller change than it sounds: the decoded audio is bit-identical, only the encoded file differs, and it is smaller. Any decoder reads either. There is no compatibility question here, only a "do you want output to move" one.
Happy to open a PR with the one-line change if useful.
This one is a bug rather than an improvement, and is independent of the two
log2replacements I have raised separately in #5..max(0.0)binds to(2.0 * std::f64::consts::LN_2), which is a positive constant — so the clamp is a no-op and the expression is justln(error * error_scale) / (2 * ln 2). Almost certainly the intent was to clamp the quotient:It matters because
error * error_scale < 1makes the logarithm negative, andbits_per_residualwith it — and that value is multiplied by the residual count insubframe_bitson the next line. So a subframe can be costed at a negative bit count during order selection, and the order search prefers whichever order drives the residual error lowest rather than the one that actually codes smallest.We applied this fix and measured it. Three of our test fixtures each came out 43 bytes smaller — about 0.3 % better compression on ~13.8 KB files — because order selection stops preferring subframes it was costing at a negative bit count. The effect is in the direction the analysis predicts.
Two caveats worth stating plainly:
Happy to open a PR with the one-line change if useful.