ruuda / hound

A wav encoding and decoding library in Rust
https://codeberg.org/ruuda/hound
Apache License 2.0
482 stars 63 forks source link

Clamp channels in WAVEFORMATEXTENSIBLE mode to avoid overflow #59

Closed diffuse closed 2 years ago

diffuse commented 2 years ago

Thank you to everyone involved in creating this awesome library!

I noticed that when creating a spec with channels > 32, an overflow occurs due to bit-shifting past the limits of the u32 type when creating the channel mask for WAVEFORMATEXTENSIBLE mode. Here is a minimal example to reproduce the behavior:

use hound;

fn main() {
    let spec = hound::WavSpec {
        channels: 33,
        sample_rate: 44100,
        bits_per_sample: 16,
        sample_format: hound::SampleFormat::Int,
    };
    hound::WavWriter::create("sine.wav", spec).unwrap();
}

After researching the spec, it mentions that:

If nChannels exceeds the number of bits set in dwChannelMask, the channels that have no corresponding mask bits are not assigned to any physical speaker position. In any speaker configuration other than KSAUDIO_SPEAKER_DIRECTOUT, an audio sink like KMixer (see KMixer System Driver) simply ignores these excess channels and mixes only the channels that have corresponding mask bits.

It also mentions that:

Channel locations beyond the predefined ones are considered reserved.

Since there are 18 predefined channel locations, I clamped the channels param to 0-18 to both ignore channels that don't have a corresponding mask bit, and to avoid writing to reserved bits.


I opened an issue for this, #60, but it is also worth noting that this method of bit shifting does not generate the same predefined masks that are detailed by the spec. I'm not sure if this was intentional, but I left this behavior in up to 18 channels to preserve this choice.

ruuda commented 2 years ago

Thanks! I merged this as 77e23ef330ad986a309e40c785ea521d3295e548.

but it is also worth noting that this method of bit shifting does not generate the same predefined masks that are detailed by the spec

Yeah, I guess the proper solution would be to allow specifying the channel mask, instead of making assumptions about it. But that would be a more invasive change.