Closed Cocalus closed 4 years ago
Thanks for the PR!
I can confirm this does fix the FLAC amplitude behaviour on my end too, even with the sine example. I'm yet to read properly through the code changes yet but just leaving this as a note for myself/others in case someone beats me to it.
I recently ran into the same issue. This PR looks fine, altough I'd suggest adding a simple test:
diff --git a/tests/read.rs b/tests/read.rs
index b7a367c..46ae39c 100644
--- a/tests/read.rs
+++ b/tests/read.rs
@@ -68,3 +68,26 @@ fn open_and_read_samples() {
// Ogg Vorbis is lossy.
read_samples(OGG_VORBIS);
}
+
+
+#[test]
+fn open_and_read_as_f32() {
+ fn read_min_max<P>(path: P) -> (f32, f32)
+ where
+ P: AsRef<std::path::Path>,
+ {
+ let mut reader = audrey::open(path).unwrap();
+ let samples = reader.samples::<f32>().map(Result::unwrap).collect::<Vec<f32>>();
+ let max_sample = samples.iter().cloned().fold(std::f32::NAN, f32::max);
+ let min_sample = samples.iter().cloned().fold(std::f32::NAN, f32::min);
+ (min_sample, max_sample)
+ }
+
+ let (wav_min, wav_max) = read_min_max(WAV);
+ assert!(wav_min < -0.5);
+ assert!(wav_max > 0.5);
+
+ let (flac_min, flac_max) = read_min_max(FLAC);
+ assert!(flac_min < -0.5);
+ assert!(flac_max > 0.5);
+}
Ideally the claxon crate would take care of this shifting, but idk. We can have the patch for as long as claxon has the buggy behaviour.
I ran into very quiet audio again with Flac this time. For the same reason that the bit depth wasn't being corrected. This was with a standard 16bit Flac. This should support any bit depth up to 32. I added errors for Wav and Flac such that unsupported bit depths will return errors. So up to 32 bits for Flac and 8,16,24,32 for Wav. While Wav has a bits per sample, everything seems to turn that into bytes per sample, so I don't think there are other valid Wav formats. But if such formats exist they will stop working with this (previously they would have their bit depth scaled incorrectly).
For spot checking code It looks like the other formats convert the bit depth internally in their respective crates, but I haven't tested them explicitly.