total_duration not working with symphonia decoders

[yara-blue]

Moving from: #190 (comment)

summary:
@gulis1 offerd to implement total_duration, I noted it should already be there for symphonia. Then they noted that is not working which I verified locally. Finally I invited them to have a go at fixing it if they would like to and pointed to roderick planning improvements to that piece of code.

[roderickvd]

Copying from there:

Yes. Beyond the feature flags, the audio specifications can be None for some codecs until the first packet is decoded. So that needs fixing. I’ll take it along with my refactoring.

Could you contribute that test? Long time since I did some TDD 😉

[yara-blue]

Made a proper integration test, see the branch tests/total_duration: https://github.com/RustAudio/rodio/tree/tests/total_duration.rs If whomever picks this up branches from that we will eventually get the test in main. (I've not merged it to main since then CI breaks for all PR's or we have to mark the test as ignored)

[gulis1]

Hello! Thanks for tests. I've been investigating the issue with vorbis files.

Unlike other formats, the headers of a vorbis file don't contain the total duration of the audio. To get the duration it is needed to seek through the whole file, in order to find the last vorbis frame. For large files this can take a while, so Symphonia makes an which needs the total size in bytes of the vorbis content.

The reason why it fails is because (ReadSeekSource) does not know the actual byte size of the audio content, so Symphonia's can't perform the optimized total_duration operation.

I've managed to get total_duration working locally for vorbis files by adding a size field to (ReadSeekSource):

pub struct ReadSeekSource<T: Read + Seek + Send + Sync> {
    inner: T,
    size: Option<usize>
}

With this however, we would need to change a bit the constructors for the Decoder struct. For example, to play files from disk, we could create a new from_file(file: fs::File) constructor, and take the byte size from the file's metadata.

Let me know what you guys think about this, and I would be happy to submit a PR with the changes

[roderickvd]

You are right in your analysis. ReadSeekSource definitely needs to have a way to have byte_len set.

Beyond files there are different ways of supplying a source stream with a known length (in particular, a HTTP stream). And then there are more Symphonia knobs that users might like to turn, or rely on defaults. Therefore I was thinking about making it a builder pattern.

[yara-blue]

Beyond files there are different ways of supplying a source stream with a known length (in particular, a HTTP stream). And then there are more Symphonia knobs that users might like to turn, or rely on defaults. Therefore I was thinking about making it a builder pattern.

Idea, we could add with_total_duration to source that modifies any duration from the stream/file. Could be useful for webstreams that have a known total_duration but are still in progress.

I agree that a builder could be a good here.

[roderickvd]

First proposal for a builder is over at #697.

It doesn't have a with_total_duration yet, because I thought to get it from the decoder, but it could be added. It does have the byte_len that should let Symphonia determine the total_duration for Vorbis files too.

[roderickvd]

Made a proper integration test, see the branch tests/total_duration: https://github.com/RustAudio/rodio/tree/tests/total_duration.rs If whomever picks this up branches from that we will eventually get the test in main. (I've not merged it to main since then CI breaks for all PR's or we have to mark the test as ignored)

I merged in that test branch over at #697.