fix(wasapi): shift i24 to/from MSB - #1309
Conversation
c81d134 to
78e4848
Compare
|
Thanks, this is a much simpler and most welcome direction than #1305. I'm doubtful whether it's correct though. Were you able to test by ear? // Simulates what WAVEFORMATEXTENSIBLE does with wValidBitsPerSample < wBitsPerSample
fn left_justify(buffer: &mut [u8]) {
for c in buffer.chunks_exact_mut(4) {
let v = u32::from_ne_bytes(c.try_into().unwrap()) << 8;
c.copy_from_slice(&v.to_ne_bytes());
}
}
fn main() {
let samples: [i32; 2] = [-1, 5];
let mut device_buf: Vec<u8> = samples.iter().flat_map(|s| s.to_ne_bytes()).collect();
left_justify(&mut device_buf);
let mut captured = device_buf.clone();
captured.copy_within(1.., 0);
let got: Vec<i32> = captured
.chunks_exact(4)
.map(|c| i32::from_ne_bytes(c.try_into().unwrap()))
.collect();
println!("expected: {:?}", samples);
println!("got: {:?}", got);
}
// expected: [-1, 5]
// got: [16777215, 5]Would something like this work instead? // render
for c in buffer_slice.chunks_exact_mut(4) {
let v = i32::from_ne_bytes(c.try_into().unwrap()) << 8;
c.copy_from_slice(&v.to_ne_bytes());
}
// capture
for c in slice::from_raw_parts_mut(buffer, byte_count).chunks_exact_mut(4) {
let v = i32::from_ne_bytes(c.try_into().unwrap()) >> 8;
c.copy_from_slice(&v.to_ne_bytes());
}Question: the capture one still writes into the buffer |
Yes, and it worked fine
Sry, I don't understand this part. Can you rephrase please?
16777215 in an i24 overflows, and wraps around to -1. So your example is actually showing correct behaviour
This is a good point. I'll fix that |
Double-checking: did your by-ear test include capture/recording specifically, or mostly render? If I'm right at all, this would be a capture issue.
That'd be right if we went through
After |
It includes both. I use the feedback example for testing
Crap, you're right, I missed that. And I guess my test just happened to work because I only tested conversion between integer formats. I'll try to test with a float conversion next time
Looks like my understanding of |
|
Not strictly UB but wrong for sure. #1305 described well how it sounded. |
|
tested with this code: use std::sync::mpsc;
use std::thread;
use cpal::{I24, Sample, SampleFormat};
use cpal::traits::{DeviceTrait, HostTrait, StreamTrait};
type SampleTypeIn = I24;
type SampleTypeOut = i32;
fn main() {
let host = cpal::default_host();
let dev_in = host.default_input_device().unwrap();
let dev_out = host.default_output_device().unwrap();
let cfg_in = dev_in .supported_input_configs() .unwrap().find(|cfg| cfg.max_sample_rate() == 48000 && cfg.sample_format() == SampleFormat::I24).unwrap().with_max_sample_rate();
let cfg_out = dev_out.supported_output_configs().unwrap().find(|cfg| cfg.max_sample_rate() == 48000 && cfg.sample_format() == SampleFormat::I32).unwrap().with_max_sample_rate();
let (sender, receiver) = mpsc::channel();
let err_cb = |error| panic!("{error}");
let stream_in = dev_in.build_input_stream(
cfg_in.into(),
move |data: &[SampleTypeIn], _| {
let array =
<[SampleTypeIn; 960]>::try_from(data)
.unwrap()
.map(f32::from_sample);
sender.send(array).unwrap();
},
err_cb,
None
).unwrap();
let stream_out = dev_out.build_output_stream(
cfg_out.into(),
move |data: &mut [SampleTypeOut], _| {
if let Ok(array) = receiver.try_recv() {
data[..960].copy_from_slice(&array.map(SampleTypeOut::from_sample));
}
else {
data.fill(SampleTypeOut::EQUILIBRIUM);
}
},
err_cb,
None
).unwrap();
stream_in.start().unwrap();
stream_out.start().unwrap();
thread::park();
}I tried all sorts of sample format combinations, and it always worked fine |
|
🙏 Question though, not out of vanity but seeking to understand, why not my earlier snippet like: // render, in place
for c in buffer_slice.chunks_exact_mut(4) {
let v = i32::from_ne_bytes(c.try_into().unwrap()) << 8;
c.copy_from_slice(&v.to_ne_bytes());
}
// capture, into scratch_buffer
for (dst, src) in scratch_buffer.chunks_exact_mut(4).zip(
slice::from_raw_parts(buffer, byte_count).chunks_exact(4)
) {
let v = i32::from_ne_bytes(src.try_into().unwrap()) >> 8;
dst.copy_from_slice(&v.to_ne_bytes());
}If this works all the same, it's simpler and prevents Two smaller ones:
|
Because my brain was in tunnel view and refused to process your messages properly 😅 Feeling a bit clearer in the head now, your use of ne_bytes does seem more correct, as my solution assumes little endian implicitly. I've also learned about arithmetic vs logical shift, and now understand why this works correctly. However, calling
Valid. I changed the
Fixed. |
|
oh, there's a merge conflict, 1 sec |
b35aa5f to
50a9f58
Compare
|
Rebased. It was just the changelog |
50a9f58 to
0363180
Compare
Ah, I see now that this was based on |
|
Do you want me to make a second PR for master ? |
|
Sure, that'd be great, once we've got this one ticked off. |
(cherrypicked from commit 684e31d)
A WAVEFORMATEXTENSIBLE describes a sample with two numbers: wBitsPerSample, the container, and wValidBitsPerSample, how much of it the sample fills. When they differ the valid bits sit at the *top* of the container -- "the valid bits (the actual PCM data) are left-aligned within the container", as ksmedia.h's WAVEFORMATEXTENSIBLE reference puts it. CPAL's I24 is the other way round: a dasp_sample::I24 is an i32 holding -(1 << 23)..=(1 << 23) - 1, at the bottom of its four-byte container. So the backend handed a 24-in-32 device every sample 2^8 too small and read every sample back 2^8 too large. It reaches both share modes: supported_formats reports I24 as supported, and the two fields go to the engine's converter in shared mode and to the driver in exclusive mode alike. Measured on a PreSonus AudioBox 22VSL (USB, Windows 11) at 24-in-32 / 48 kHz stereo, in both share modes and both directions, converting I24 to F32 on input and F32 to I24 on output. Output was 48 dB too quiet -- clean, just far below level -- and input pinned to full scale by anything above near-silence, moving only when the source was nearly silent. Instrumented on the capture side, a shared-mode I24 stream returned samples 2^8 too large, within 0.6 dB of an I32 reference on the same scale. Render shifts up in place, between the data callback and ReleaseBuffer: that buffer is the backend's to write until it is handed back. Capture cannot do the same, since GetBuffer lends WASAPI's own packet and lends it to be read, so the samples are shifted down into a staging buffer on their way to the callback -- sized once at stream build, so the callback still allocates nothing. It is a Vec<i32> rather than a Vec<u8> because that buffer reaches the callback as a Data, whose as_slice casts to the sample type, and a Vec<u8> guarantees no alignment. The shift is wBitsPerSample - wValidBitsPerSample read off the format handed to Initialize, not a test for I24, so every format whose container is exactly full -- I16, I32, F32 -- lands on zero by the same arithmetic and keeps its existing zero-copy path byte for byte. A container that is padded but not four bytes wide is refused rather than passed through unshifted, which would be silently wrong by the width of the padding. The integer half is a platform-neutral module so it can be tested off Windows: the round trip, the -2^23 and 2^23 - 1 boundaries, the sign carried down, padding bits discarded, the shift actually coming from the format, a trailing partial container left alone, and 16- and 32-bit formats passed through untouched. Rebased onto a master that has since landed its own I24 fix (RustAudio#1309, RustAudio#1311), which shifts by a hardcoded 8 whenever the sample format is I24. That special case is replaced here, not duplicated: the shift is read off the negotiated format's own wBitsPerSample and wValidBitsPerSample, which is 8 for the one padded pair the backend can encode, so the numbers are unchanged. The capture staging buffer moves off run_input's stack onto StreamInner. On capture the shift moved behind the draining gate the callback already sits behind; on render it stays outside that gate, so the equilibrium a drain writes is justified into the device's domain like any other sample. Measured on hardware before this rebase, not since. The implementation those measurements describe is not the one in this commit, and the rebased code has not been run on Windows: the WASAPI tests compile for the MSVC target and were never executed. Unverified.
A WAVEFORMATEXTENSIBLE splits a sample into wBitsPerSample, the container, and wValidBitsPerSample, the sample itself; where they differ the sample sits at the top of the container. CPAL's I24 sits at the bottom of its i32, so the backend handed a 24-in-32 device every sample 2^8 too small and read every one back 2^8 too large, in both share modes and both directions. Measured on a PreSonus AudioBox 22VSL: output 48 dB down, input pinned to full scale. The shift is the difference between the format's own two bit counts rather than a test for I24, so a container that is exactly full lands on zero and keeps its zero-copy path. The arithmetic is a platform-neutral module, testable off Windows. Master has since landed its own I24 fix (RustAudio#1309, RustAudio#1311), hardcoding a shift of 8 whenever the format is I24. That special case is replaced rather than duplicated: 8 is what the arithmetic yields for the one padded pair the backend can encode, so the numbers are unchanged. Measured on hardware before this rebase, not since, and not run on Windows in this form. Unverified.
2 participants
supersedes #1305