audio/wav: accept more audio formats 24bit #3024
Conversation
audio/wav/decode.go
Outdated
| if bitsPerSample != 8 && bitsPerSample != 16 { | ||
| return nil, fmt.Errorf("wav: bits per sample must be 8 or 16 but was %d", bitsPerSample) | ||
| if !convert.IsValidResolution(bitsPerSample) { | ||
| return nil, fmt.Errorf("wav: invalid bits per sample must be [8,16,24,32] but was %d", bitsPerSample) |
There was a problem hiding this comment.
Adding "invalid" here makes this error message sound unnatural. Better message: "wav: bits per sample must be 8, 16, 24, or 32, but was %d"
audio/wav/decode.go
Outdated
| case 2: | ||
| mono = false | ||
| case 1, 2: | ||
| mono = channelCount == 1 |
There was a problem hiding this comment.
IMO this new code is less clear and less explicit than before.
There was a problem hiding this comment.
Your codebase. Your call. :)
audio/internal/convert/stereo.go
Outdated
| "io" | ||
| ) | ||
|
|
||
| // Setero objects convert little-endian audio mono or stereo audio streams into |
audio/internal/convert/stereo.go
Outdated
| // IsValidResolution returns true if the given bit resolution is supported. | ||
| func IsValidResolution(r int) bool { | ||
| switch r { | ||
| case 8, 16, 32, 24: |
There was a problem hiding this comment.
32bit is not the target. (I think 32bit can be float values https://www.sounddevices.com/32-bit-float-files-explained/)
There was a problem hiding this comment.
I see references to both PCM and float. It seems there's lots of abuse of the WAV file format. In any case, I'll eliminate it from this change.
| b[2*i+0] = s.buf[m*(i+1)-2] | ||
| b[2*i+1] = s.buf[m*(i+1)-1] | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this really work with 24bits?
There was a problem hiding this comment.
If you look at my test case, it's just throwing away the low 8 bits of resolution. See comments in the test file to follow up, please.
| {16, true, nil, res16}, | ||
| {16, false, nil, res16}, | ||
| {24, true, nil, res16}, | ||
| {24, false, nil, res16}, |
There was a problem hiding this comment.
AFAICT, the whole point of Stereo is that it converts 8/16[/24/32] bit samples into 16 bit stereo samples. I was just keeping the same functionality as previous. If that understanding is correct, then 16bit values are the gold standard. Now that doesn't work for 8bit samples, as we just don't have the low-bits of resolution, but I'm just trying to create the same functionality.
Incidentally, 2 small notes:
-
16 bits of resolution is 96db of SNR (at full scale). That should be plenty, and using 16bits throughout the sound system is probably adequate.
-
The original conversion code looks incorrect to me. For example, the 8bit code duplicated the byte high and low (constant
0x101). While that's a solution, I think it's less surprising than just calling the low-bits 0.
There was a problem hiding this comment.
AFAICT, the whole point of Stereo is that it converts 8/16[/24/32] bit samples into 16 bit stereo samples.
Yes that's correct. Stereo16 (the original name) indicates that the stereo 16bit (little endian) PCM source, whatever the original source is. So, 8bit code is multiplied by 0x101 to convert 8bit to 16bit values.
Please revert your renaming (Stereo16 to Stereo).
Now that doesn't work for 8bit samples, as we just don't have the low-bits of resolution, but I'm just trying to create the same functionality.
Do you mean your current test res8 doesn't work, or the current implementation (before your PR) doesn't work?
If we think the current implementation doesn't work with 8bit wav, we should focus on this problem first.
There was a problem hiding this comment.
I've confiemd 8bits worked correctly with the current implementation. 1cd6a1f It's because samples in 8bit wav file are unsigned, not signed.
Then, please revert your implementation for 8bits. Thanks,
jeremyfaller
changed the title
What issue is this addressing?
Closes #2215
What type of issue is this addressing?
Bug/Feature request
What this PR does | solves
Adds support for 24 and 32 bit audio files.