Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculate duration for mp3s #481

Closed
wants to merge 4 commits into from

Conversation

kuviman
Copy link
Contributor

@kuviman kuviman commented Feb 19, 2023

Currently, the duration is not calculated for mp3s when using symphonia

let old_pos = stream.stream_position()?;
let len = stream.seek(SeekFrom::End(0))?;

// Avoid seeking a third time when we were already at the end of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a third time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the source: https://github.com/rust-lang/rust/blob/master/library/std/src/io/mod.rs#L1841

Third time is needed to go back to initial position

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, i guess its only 2 seeks, so its second, not third, bug in a comment in std I guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, stream_position() is also calling a seek, so it is indeed a third

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three seeks are a lot, and it seems that's also the reason why the function isn't stable yet. Isn't there some API that symphonia provides to optionally return the length of a stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you asking about MediaSource::byte_len method? Yes, it is provided by symphonia, and this PR changes the implementation to return Some calculated length now instead of returning None as before. This is then used to estimate the duration of the track

The method says that "This may be an expensive operation"

Although in my implementation here, I cache it when creating the source instead of calculating when the method is called.
I did it because of the method requiring shared reference to self instead of a &mut reference.

This could instead be done with a RefCell so this "expensive" operation is performed only when actually needed

Would that be better?

Other ways I see is changing the API bringing a trait like MediaSource into rodio itself. so that implementation of this is defined by the user, but I don't think thats a good idea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the RefCell idea tbh.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also make total_duration() work for all symphonia-decoded audio, which would be really helpful for my projects. @kuviman if you want me to try make the refcell changes, I'd be happy to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the PR to use lazy calculation. This is done not using RefCell, but with a Mutex/RwLock since we need the source to by Sync.

This should not have any performance issues tho since get_mut is enough and works without locking for normal usage, and locking will only happen once when we need to calculate the stream length

@ryankopf
Copy link

Will someone please merge this.

@dvdsk
Copy link
Collaborator

dvdsk commented Apr 10, 2024

First off all, sorry this took a while we are short on maintainers. Thanks for making a PR though! Part of what you did in this PR overlaps with #513 (which has been merged).

This:

use std::io::BufReader;

use rodio::Source;

fn main() {
    let (_stream, handle) = rodio::OutputStream::try_default().unwrap();
    let sink = rodio::Sink::try_new(&handle).unwrap();

    let file = std::fs::File::open("assets/music.mp3").unwrap();
    let source = rodio::Decoder::new(BufReader::new(file)).unwrap();
    dbg!(source.total_duration());
    sink.append(source);

    sink.sleep_until_end();
}

prints:

[examples/music_mp3.rs:11:5] source.total_duration() = Some(
    10.000000006s,
)

Which is correct for the music.mp3 test file.

And that's good news! That means we get a duration for mp3 files without introducing the overhead of implementing bytes_len().

@kuviman if I am wrong and total_duration is not working for you on the latest master branch commit please reopen this or let me known in some other way!

@dvdsk dvdsk closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants