fix(wasapi): shift i24 to/from MSB

[LastExceed]

supersedes #1305

[roderickvd]

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?copy_within moves bytes across the whole buffer, so on more than one frame won't work. Minimal example showing what seems to be the mismatch:

// 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 GetBuffer handed, and formally that's read-only. Not sure if that works in practice? May work fine, but for correctness should we use a scratch buffer there?

[LastExceed]

Were you able to test by ear?

Yes, and it worked fine

so on more than one frame won't work

Sry, I don't understand this part. Can you rephrase please?

// expected: [-1, 5]
// got: [16777215, 5]

16777215 in an i24 overflows, and wraps around to -1. So your example is actually showing correct behaviour

the capture one still writes into the buffer GetBuffer handed, and formally that's read-only. Not sure if that works in practice? May work fine, but for correctness should we use a scratch buffer there?

This is a good point. I'll fix that

[roderickvd]

Yes, and it worked fine

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.

16777215 in an i24 overflows, and wraps around to -1. So your example is actually showing correct behaviour

That'd be right if we went through I24::from(), which would wrap, but we don't go through that. Data::as_slice::<I24>() just takes from raw memory anddasp_sample's conv.rs says that conversions do not check the range of incoming values for I24. So 16777215 doesn't become -1; instead, supposing we'd convert to f32 sample it'd become 16777215.0 / 8388608.0 ~= 2.0 rather than ~0.0.

Sry, I don't understand this part. Can you rephrase please?

After copy_within(1.., 0), byte index 3 (the last byte of frame 0) gets whatever was at index 4, which is frame 1's own leading (padding) byte, not anything from frame 0. So frame 0's sign-extension byte is borrowed from its neighbor. That borrowed byte happens to be 0, which is coincidentally right for positive samples but wrong for negative ones.

[LastExceed]

did your by-ear test include capture/recording specifically, or mostly render?

It includes both. I use the feedback example for testing

That'd be right if we went through I24::from(), which would wrap, but we don't go through that.

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

After copy_within(1.., 0), byte index 3 (the last byte of frame 0) gets whatever was at index 4, which is frame 1's own leading (padding) byte

Looks like my understanding of I24's in-memory representation was wrong. I had assumed that the 4th byte is arbitrary. But then this also means that blindly casting the raw memory to &[I24] was already UB before my change, since WASAPI can put whatever it wants into the 4th byte, right?

[roderickvd]

Not strictly UB but wrong for sure. #1305 described well how it sounded.

[LastExceed]

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

[roderickvd]

🙏

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 byte_count - 1 overflowing when byte_count == 0.

Two smaller ones:

• scratch_buffer.chunks_mut(4) walks the buffer every callback, not just the byte_count bytes actually reported this call. Using the above or slicing to &scratch_buffer[..byte_count] would fix that.
• scratch_buffer gets allocated for every capture stream regardless of format, when we only need it if sample_format == SampleFormat::I24.

[LastExceed]

why not my earlier snippet

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 i32::from_ne_bytes(src.try_into().unwrap()) on every sample still itches me, so I looked for an alternative, and it occurred to me that we can just treat the scratch buffer as [i32] from the start.

scratch_buffer.chunks_mut(4) walks the buffer every callback, not just the byte_count bytes actually reported this call

Valid. I changed the .copy_from_slice() to Vec::clear() + .extend_from_slice() for this. This is just as cheap (apart from updating the length value 2x), because the Vec internally retains its capacity, but a lot more readable IMO, and it automatically grows the scratch buffer, should the need arise.

scratch_buffer gets allocated for every capture stream regardless of format, when we only need it if sample_format == SampleFormat::I24.

Fixed.

[roderickvd]

😅 last steps now.

[LastExceed]

oh, there's a merge conflict, 1 sec

[LastExceed]

Rebased. It was just the changelog

[roderickvd]

Rebased. It was just the changelog

Ah, I see now that this was based on develop instead of master. We should include this in 0.18.2 but it doesn't matter which way we port, from master to develop or the other way around.

[LastExceed]

Do you want me to make a second PR for master ?

[roderickvd]

Sure, that'd be great, once we've got this one ticked off.