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

Add support for reading/writing S24_LE samples #40

Closed fletcherw closed 4 years ago

fletcherw commented 4 years ago

It appears that hound does not allow reading wav files whose bit depths differ from their container sizes. One example of such audio is S24_LE (3 byte samples stored in 4 bytes). The line that prevents this is, I believe: https://github.com/ruuda/hound/blob/master/src/read.rs#L480, since it assumes that bits per sample equals the block alignment * 8 / number of channels. However, for S24_LE, this is not actually the case.

To see this, you can create a S24_LE recording with 'arecord -f S24_LE input.wav', and then examine the metadata with 'shntool info input.wav':

File name: input.wav Handled by: wav format module Length: 1491:18.485 WAVE format: 0x0001 (Microsoft PCM) Channels: 1 Bits/sample: 24 [...] Block align: 4 [...]

I think this could be handled by adding a function to read a S24_LE sample out of 4 bytes, and to discard the empty byte.

A similar problem exists with writing S24_LE samples, since there is no way for the user to specify the bytes_per_sample in WavFormatEx. I think it could maybe support this if WavSpec allowed specifying not just bit_depth but also the block_align.

What are your thoughts? If you want to decide on a preferred approach, I'd be happy to submit a pull request for this.

ruuda commented 4 years ago

Thank you for taking the time to open an issue.

I think this could be handled by adding a function to read a S24_LE sample out of 4 bytes, and to discard the empty byte.

Sample::read supports this, it takes a number of bytes to read, and the number of bits in the sample. What would need to be added is a case here for 4 bytes but 24 bits.

I think all what needs to happen for reading, is to relax that check that you mentioned, and to pass that down to the bytes_per_sample in WavSpecEx.

For writing, we could add an alternative constructor that accepts a WavSpecEx, and then forward the current constructor to that, deriving the WavSpecEx from the WavSpec like here. We could add a new method Sample::write_padded that takes a number of bytes and bits, like read, and forward write to that, passing bytes = bits / 8.

If you want to decide on a preferred approach, I'd be happy to submit a pull request for this.

What would be very useful, is if you could add a tiny example file in this S24_LE format, with a few prepared, known samples, and to add a test for it. There is a collection of these files already in testsamples/, and tests that read them here.

fletcherw commented 4 years ago

Tests, as well as a patch to get one of the cases working, are here: #41. Thanks for your quick response!

Let me know if there's some other way I can help!

fletcherw commented 4 years ago

Closing since #41 and #42 have been merged.