Skip to content

Operator-precedence bug at encode.rs:3675: .max(0.0) clamps a constant, not the quotient #6

Description

@ztrencsenyi

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions