Fix span length issues in queue

[dvdsk]

Adjust Source docs: span must be whole frames. This was already required by the various parts of rodio but never mentioned anywhere.

Rodio was using a fixed span_length for all sources regardless of their number of channels. If that span length was not wholly dividable by the number of channels it
would create an offset breaking channel volume.

Edit: credit for finding this goes to @will3942

[dvdsk]

I think this fixes #484 can anyone test?

[will3942]

I think this fixes #484 can anyone test?

Testing now!

[will3942]

@dvdsk This is still broken for a 6 channel output device - works fine for a 4 channel and a 2 channel device.

It's broken in a different way to before where instead of outputting on channels 5 and 6 it outputs on channels 3 and 4.

Screenshots show working 4 channel and broken 6 channel.

[PetrGlad]

I think this is the right change, but likely incomplete.

Does Queue verify that input/output channel counts always match (for every source in input queue)? I may have missed it, but there seems to be no checks in this regard (channel count can also change on the next span).
I see that it just always matches the current source.
Maybe problem remains when new input source is added over silence...

There's // TODO: not yet implemented since 2017 (src/queue.rs:313) :)

[PetrGlad]

Inline, maybe.

[dvdsk]

Maybe problem remains when new input source is added over silence...

good point, gotta check that the channels are still in sync then. We should probably check other parts of the library. From a quick glace it seems skippable has the same issue. So there is a wider problem with multi channel audio and span length. For now I would say pad everything with silence. Thats gonna give different issues (silence between sounds) but we can tackle those afterwards.

[will3942]

This looks good in theory but having reviewed this morning it's flawed due to self.channels() reporting the channels for the current source which in the case of silence or Empty (which appears to cause the issue with multichannel audio) is 1 and not the number of channels of the remaining sources or the output device.

In my head, the actual fix seems like it could be quite tricky as do we need to know the number of output channels on the Mixer to be able to fix this properly?

I have been able to implement a "fix" in our software for a 6 channel device by using this PR's span length fix and then setting Empty's channels to 6 and where we create silence here using Zero, setting channels to 6 but this is by no means a proper fix or reusable.

Edit: This is actually just a partial "fix" as due to what @dvdsk has said, skippable has the same issue and so when we use sink.clear() depending on when this is called and the samples are skipped we end up shifting the samples / channel ordering.

[will3942]

self.channels() will return 1 when we're either playing silence or at the start of the queue with the Empty source.

[dvdsk]

we should rename current_span_len() to samples_left_in_span() or something better. The current name sounds like the length is fixed, it can however be called at any time and then should returns the samples left until the resample rate and/or channel count can change. That is better communicated by having the word "left" in the name. Adding the word "samples" to the name makes it clear its not the number of frames.

[dvdsk]

@will3942 its worse, I took a deeper look and there are about 5 edge cases not handled by queue. I'll have to rewrite it completely. Gonna be a fun review for you all.

[will3942]

@will3942 its worse, I took a deeper look and there are about 5 edge cases not handled by queue. I'll have to rewrite it completely. Gonna be a fun review for you all.

eek - thanks for your time on this.

[dvdsk]

ready for review

[dvdsk]

@will3942 does this pass your test (note skip is still broken). If it does not we really need to work on getting an automated integration test for this issue.

[dvdsk]

Need to also handle skip & skip_duration.... Since its perfectly fine to end mid frame or before the span ends. So not done yet :(

[will3942]

@will3942 does this pass your test (note skip is still broken). If it does not we really need to work on getting an automated integration test for this issue.

I don't have a great test bed for this right now - will test this tomorrow.

[PetrGlad]

Maybe just "Queue"? This would be more in line with other control/source struct pairs.

[will3942]

@dvdsk sadly this does not fix the issue.

I am using the following code to test it - interestingly if you note the commented out code, when using a SamplesBuffer the code works correctly and outputs on Channels 1 and 2 however using a Decoder results in an output on Channels 5 and 6 - this might help being able to build up an integration test that simulates this as I believe when trying to build tests for this I was only using SamplesBuffer.

use std::{error::Error, io::BufReader};

use cpal::traits::HostTrait;
use rodio::{buffer::SamplesBuffer, source::ChannelVolume, DeviceTrait};

fn main() -> Result<(), Box<dyn Error>> {
    // Find device named "Loopback Audio" and use it
    let mut devices = cpal::default_host().output_devices()?;
    let debug_devices = cpal::default_host().output_devices()?;

    println!("Devices: {:?}", debug_devices.map(|device| device.name()).collect::<Vec<_>>());

    let device = devices
        .find(|device| device.name().unwrap().to_lowercase().contains("loopback"))
        .unwrap();

    let stream_builder = rodio::OutputStreamBuilder::from_device(device)?;
    let stream_handle = stream_builder.open_stream()?;
    let sink = rodio::Sink::connect_new(&stream_handle.mixer());

    let file = std::fs::File::open("assets/test-bad-seek.mp3").unwrap();
    let decoder = rodio::Decoder::new(BufReader::new(file)).unwrap();
    let channel_volume = ChannelVolume::new(decoder, vec![1.0, 1.0, 0.0, 0.0, 0.0, 0.0]);

    // let six_channel_buffer = SamplesBuffer::<i16>::new(
    //     6,
    //     48000,
    //     vec![20000i16, 20000, 0, 0, 0, 0].into_iter().cycle().take(48000 * 100).collect::<Vec<i16>>(),
    // );

    // let channel_volume = ChannelVolume::new(six_channel_buffer, vec![1.0, 1.0, 0.0, 0.0, 0.0, 0.0]);

    sink.append(channel_volume);

    sink.sleep_until_end();

    Ok(())
}

[dvdsk]

EDIT: found more edge cases :) Resolving them, ill let you know when testing is needed again

I tried to make a test, however it keeps succeeding. I do not have your test-bad-seek.mp3 file so maybe its a file specific thing or maybe I am not simulating sink correctly.

Could you check if:

• your manual test fails with music.mp3
• it fails if you do not use sink?

For that last setup you will need to do:

    let stream_handle = rodio::OutputStreamBuilder::open_default_stream()?;
    let mixer = stream_handle.mixer();
    mixer.add(your source);

    sleep a few seconds then exit program

[dvdsk]

rebase^

[will3942]

@dvdsk This is the file if needed for future ref.

test-bad-seek.mp3.zip

[dvdsk]

test pass with that file

test:

#[test]
fn with_queue_in_between() {
    let file = fs::File::open("assets/test-bad-seek.mp3").unwrap();
    let decoder = Decoder::new(BufReader::new(file)).unwrap();
    assert_eq!(decoder.channels(), 2);
    let channel_volume = ChannelVolume::new(decoder, vec![1.0, 1.0, 0.0, 0.0, 0.0, 0.0]);
    assert_eq!(channel_volume.channels(), 6);

    let (controls, queue) = queue::queue(false);
    controls.append(channel_volume);

    assert_output_only_on_channel_1_and_2(queue);
}

It does take quite a lot of time on a release build.... which scares me. But lets first get it working before we refactor and finally optimize.

@will3942 could you take the latest version of this pr and run your manual test? I think the test above should match Sink. We are not resampling but that should not matter.

[will3942]

@dvdsk This latest version works perfectly on my example test!