issues with ogg delay and padding with gapless

[bobbens]

I've noticed clicking when looping vorbis audio. To determine the cause, I've loaded the PCM data with symphonia and written it to disk, to compare with PCM data written from ffmpeg. I've tried different formats to see what is causing the gaps to appear. Results are below:

# Reference generated with ffmpeg
-rw-r--r-- 1 ess ess 94592 Dec 17 09:38 ref.pcm # ffmpeg conversion to PCM with '-f f32le -acodec pcm_f32le'
# Below generated after loading with symphonia
-rw-r--r-- 1 ess ess 94592 Dec 17 10:09 test_flac.pcm # original flac lossless data
-rw-r--r-- 1 ess ess 94592 Dec 17 10:09 test_ogg_flac.pcm # ogg+flac from encoding flac with '-c:a flac -f ogg'
-rw-r--r-- 1 ess ess 98048 Dec 17 10:05 test_ogg_vorbis.pcm # ogg+vorbis from encoding flac with ogg defaults

enable_gapless is always set.

I don't have a minimal test case, but the relevant loading code is:

enum Frame<T> {
    Mono(T),
    Stereo(T, T),
}
impl<T> Frame<T> {
    /// Loads a Vec of Frame<f32> from an AudioBufferRef
    fn load_frames_from_buffer_ref(
        buffer: &symphonia::core::audio::AudioBufferRef,
    ) -> Result<Vec<Frame<f32>>> {
        fn load_frames_from_buffer<S: Sample>(
            buffer: &symphonia::core::audio::AudioBuffer<S>,
        ) -> Result<Vec<Frame<f32>>>
        where
            f32: FromSample<S>,
        {
            match buffer.spec().channels.count() {
                1 => Ok(buffer
                    .chan(0)
                    .iter()
                    .map(|sample| Frame::Mono((*sample).into_sample()))
                    .collect()),
                2 => Ok(buffer
                    .chan(0)
                    .iter()
                    .zip(buffer.chan(1).iter())
                    .map(|(left, right)| {
                        Frame::Stereo((*left).into_sample(), (*right).into_sample())
                    })
                    .collect()),
                _ => anyhow::bail!("Unsupported channel configuration"),
            }
        }
        use symphonia::core::audio::AudioBufferRef;
        match buffer {
            AudioBufferRef::U8(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::U16(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::U24(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::U32(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::S8(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::S16(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::S24(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::S32(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::F32(buffer) => load_frames_from_buffer(buffer),
            AudioBufferRef::F64(buffer) => load_frames_from_buffer(buffer),
        }
    }
}
...

        // Relevant loading code
        let codecs = &CODECS;
        let probe = symphonia::default::get_probe();
        let mss = MediaSourceStream::new(Box::new(src), Default::default());
        let mut hint = symphonia::core::probe::Hint::new();
        if let Some(ext) = ext {
            hint.with_extension(ext);
        }
        // Enable gapless so encoded padding (e.g. Opus pre-skip/end trim) is removed.
        let mut format = probe
            .format(
                &hint,
                mss,
                &symphonia::core::formats::FormatOptions {
                    enable_gapless: true,
                    ..Default::default()
                },
                &Default::default(),
            )?
            .format;

        let track = format.default_track().context("No default track")?;
        let track_id = track.id;

        let codec_params = &track.codec_params;
        let sample_rate = codec_params.sample_rate.context("Unknown sample rate")?;
        let mut decoder = codecs.make(codec_params, &Default::default())?;

        let channels = track.codec_params.channels.context("no channels")?;
        if !channels.contains(Channels::FRONT_LEFT) {
            anyhow::bail!("no mono channel");
        }
        let stereo = channels.contains(Channels::FRONT_RIGHT);

        let mut frames: Vec<Frame<f32>> = vec![];
        loop {
            // Get the next packet from the media format.
            let packet = match format.next_packet() {
                Ok(packet) => packet,
                Err(e) => match e {
                    symphonia::core::errors::Error::IoError(e) => {
                        if e.kind() == std::io::ErrorKind::UnexpectedEof
                            && e.to_string() == "end of stream"
                        {
                            break;
                        }
                        return Err(symphonia::core::errors::Error::IoError(e).into());
                    }
                    e => anyhow::bail!(e),
                },
            };

            // If the packet does not belong to the selected track, skip over it.
            if packet.track_id() == track_id {
                // Decode the packet into audio samples.
                let buffer = decoder.decode(&packet)?;
                frames.append(&mut Frame::<f32>::load_frames_from_buffer_ref(&buffer)?);
            }
        }
...

Looking into the loop in detail, it seem like the packet is not correct:

[lib.rs:352:17] packet.dur = 0
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 0
[lib.rs:352:17] packet.ts = 0
[lib.rs:352:17] packet.dur = 128
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 128
[lib.rs:352:17] packet.ts = 0
[lib.rs:352:17] packet.dur = 128
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 128
[lib.rs:352:17] packet.ts = 0
[lib.rs:352:17] packet.dur = 128
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 128
[lib.rs:352:17] packet.ts = 0
[lib.rs:352:17] packet.dur = 576
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 576
[lib.rs:352:17] packet.ts = 96
[lib.rs:352:17] packet.dur = 1024
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 1024
[lib.rs:352:17] packet.ts = 1120
[lib.rs:352:17] packet.dur = 1024
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0
[lib.rs:352:17] Frame::<f32>::load_frames_from_buffer_ref(&buffer)?.len() = 1024
[lib.rs:352:17] packet.ts = 2144
[lib.rs:352:17] packet.dur = 1024
[lib.rs:352:17] packet.trim_start = 0
[lib.rs:352:17] packet.trim_end = 0

It seems like the encoder delay is not getting trimmed with gapless_enabled automatically. Nor is information set in trim_start and trim_end.

[bobbens]

After hitting my head on this for a while, I found that I was not trimming according to codec_params.delay and codec_params.padding. The following fixes the file sizes:

        if let Some(padding) = codec_params.padding {
            frames.truncate(frames.len() - padding as usize);
        }
        if let Some(delay) = codec_params.delay {
            let delay = delay as usize;
            if delay >= frames.len() {
                anyhow::bail!(format!(
                    "Audio '{}' is draining all frames!",
                    path.display()
                ));
            }
            frames.drain(..delay);
        }

This should probably be added to the gapless play section of the documentation.

However, I've found that the sounds are still off. Analyzing the wave forms by saving to wav and analyzing with audacity I found (top is flac, bottom is ogg):

Now, my understanding is that:

delay: Option<u32>
The number of leading frames inserted by the encoder that should be skipped during playback.
padding: Option<u32>
The number of trailing frames inserted by the encoder for padding that should be skipped during playback.

I found if I flip padding and delay (that is truncate delay and drain padding), it works perfectly. So it seems like the padding and delay parameters are being inverted.

[pdeljanov]

Hmm, beyond setting enable_gapless, a user of Symphonia should not need to do anything else. The format reader should correctly populate the trim_start and trim_end fields of the Packet, and the decoder should abide by those and trim out the samples.

Sounds like a bug with the OggReader based on what you've said. Are you able to provide a broken sample file? Or is any encoding produced by ffmpeg able to repro it?

[bobbens]

Hmm, beyond setting enable_gapless, a user of Symphonia should not need to do anything else. The format reader should correctly populate the trim_start and trim_end fields of the Packet, and the decoder should abide by those and trim out the samples.

I see. What I'm doing is slurping up all the packets, the concatenating them into a single buffer (would be great if symphonia had that functionality built in, but that's for another day), and then using that with OpenAL. It seems like the padding and delay of the entire file are not being trimmed from the packets so the entire waveform gets displaced. So if I understand correctly, it should be how I originally thought enable_gapless worked.

Sounds like a bug with the OggReader based on what you've said. Are you able to provide a broken sample file? Or is any encoding produced by ffmpeg able to repro it?

Pretty much any file generated by ffmpeg has that behaviour.

Below is the test I'm using which works when I do manual padding and delay removal mentioned above (opposite of what is written in the documentation). It grabs a file, encodes it with ffmpeg as flac + opus and tests to see if loading gives the same. You can change from opus to vorbis and it gives the same issue, which leads me to believe it is the ogg container.

#[test]
pub fn test_flac_vs_opus () {
    use std::process::{Command, Stdio};

    ndata::setup().unwrap();

    // Set up audio
    let attribs: Vec<ALint> = vec![0, 0];
    let device = al::Device::new(None).unwrap();
    let context = al::Context::new(&device, &attribs).unwrap();
    context.set_current().unwrap();

    let path = Buffer::get_valid_path( "snd/sounds/nav" ).unwrap();
    let path = Path::new("../assets").join(path);

    let temp = ndata::physfs::get_write_dir();
    let outfile1 = Path::new( "naev_audio_test.flac" );
    let outfile2 = Path::new( "naev_audio_test.opus" );
    let out1 = temp.join( &outfile1 );
    let out2 = temp.join( &outfile2 );

    assert!( Command::new("ffmpeg")
        .arg("-loglevel")
        .arg("error")
        .arg("-i")
        .arg(&path)
        .arg("-y")
        .arg("-c:a")
        .arg("flac")
        .arg(&out1)
        .stdout(Stdio::null())
        .spawn().unwrap()
        .wait().unwrap().success() );

    assert!( Command::new("ffmpeg")
        .arg("-loglevel")
        .arg("error")
        .arg("-i")
        .arg(&path)
        .arg("-y")
        .arg("-c:a")
        .arg("libopus")
        .arg("-b:a")
        .arg("256k")
        .arg("-frame_duration")
        .arg("40")
        .arg("-ar")
        .arg("48000")
        .arg(&out2)
        .stdout(Stdio::null())
        .spawn().unwrap()
        .wait().unwrap().success() );

    let data1 = BufferData::from_path( &outfile1 ).unwrap();
    let data2 = BufferData::from_path( &outfile2 ).unwrap();
    assert_eq!( data1.data.len(), data2.data.len() );

    let mut err = 0.0;
    for (sa, sb) in data1.data.iter().zip( data2.data.iter() ) {
        err += (sa-sb).powf(2.0);
    }
    err /= (data1.data.len() + data2.data.len()) as f32;

    assert!( err < 1e-3 );
}

[bobbens]

I've created a repo to reproduce at https://codeberg.org/bobbens/symphonia_ogg_bug

If you compile, it will fail assertions due to file length. If you uncomment the code to remove padding and such, it works fine.

[pdeljanov]

Hi @bobbens, I have found the issue.

This seems to be an edge case with single-page OGG files where any excess samples should be considered as padding, not delay. Symphonia did the opposite. I've tested your repo before and after the change and the assertion passes.

I'm going to do some more testing on this patch first, so I'll commit it in a day or so.

[bobbens]

Hi @bobbens, I have found the issue.

This seems to be an edge case with single-page OGG files where any excess samples should be considered as padding, not delay. Symphonia did the opposite. I've tested your repo before and after the change and the assertion passes.

I'm going to do some more testing on this patch first, so I'll commit it in a day or so.

Thanks! Just to double check, with the code as is, it now passes (without the commented out code)?

[pdeljanov]

Thanks! Just to double check, with the code as is, it now passes (without the commented out code)?

Yes, the only thing I changed was updating the Cargo.toml to use a local version.

Unfortunately I ran into some regressions so it will take a bit longer to work through those.