Fix span length issues in queue

[yara-blue]

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

[yara-blue]

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.

[yara-blue]

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.

[yara-blue]

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.

[yara-blue]

@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.

[yara-blue]

ready for review

[yara-blue]

@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.

[yara-blue]

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.

[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(())
}

[yara-blue]

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

[yara-blue]

rebase^

[will3942]

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

test-bad-seek.mp3.zip

[yara-blue]

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!