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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions src/decoder/read_seek_source.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,36 @@
use std::io::{Read, Result, Seek, SeekFrom};
use std::marker::Sync;
use std::sync::{Mutex, RwLock};

use symphonia::core::io::MediaSource;

pub struct ReadSeekSource<T: Read + Seek + Send + Sync> {
inner: T,
inner: Mutex<T>,
byte_len: RwLock<Option<Option<u64>>>, // One option is for lazy calculation
}

// Copied from std Seek::stream_len since its unstable
fn stream_len(stream: &mut impl Seek) -> std::io::Result<u64> {
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

// stream. The branch is usually way cheaper than a seek operation.
if old_pos != len {
stream.seek(SeekFrom::Start(old_pos))?;
}

Ok(len)
}

impl<T: Read + Seek + Send + Sync> ReadSeekSource<T> {
/// Instantiates a new `ReadSeekSource<T>` by taking ownership and wrapping the provided
/// `Read + Seek`er.
pub fn new(inner: T) -> Self {
ReadSeekSource { inner }
ReadSeekSource {
inner: Mutex::new(inner),
byte_len: RwLock::new(None),
}
}
}

Expand All @@ -21,18 +40,31 @@ impl<T: Read + Seek + Send + Sync> MediaSource for ReadSeekSource<T> {
}

fn byte_len(&self) -> Option<u64> {
None
// Check if length was calculated before
let byte_len = self.byte_len.read().unwrap();
if let Some(cached) = *byte_len {
return cached;
}
std::mem::drop(byte_len); // Release read lock

let mut inner = self.inner.lock().unwrap();
let calculated_stream_len = match stream_len(&mut *inner) {
Ok(len) => Some(len),
Err(_) => None, // Ignore error, cache failure
};
*self.byte_len.write().unwrap() = Some(calculated_stream_len);
calculated_stream_len
}
}

impl<T: Read + Seek + Send + Sync> Read for ReadSeekSource<T> {
fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
self.inner.read(buf)
self.inner.get_mut().unwrap().read(buf)
}
}

impl<T: Read + Seek + Send + Sync> Seek for ReadSeekSource<T> {
fn seek(&mut self, pos: SeekFrom) -> Result<u64> {
self.inner.seek(pos)
self.inner.get_mut().unwrap().seek(pos)
}
}
14 changes: 13 additions & 1 deletion src/decoder/symphonia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub struct SymphoniaDecoder {
format: Box<dyn FormatReader>,
buffer: SampleBuffer<i16>,
spec: SignalSpec,
duration: Option<Duration>,
}

impl SymphoniaDecoder {
Expand Down Expand Up @@ -80,6 +81,16 @@ impl SymphoniaDecoder {
},
)?;

// Calculate duration if possible
let mut duration = None;
if let Some(time_base) = &stream.codec_params.time_base {
if let Some(n_frames) = stream.codec_params.n_frames {
let time = time_base.calc_time(n_frames);
duration =
Some(Duration::from_secs(time.seconds) + Duration::from_secs_f64(time.frac));
}
}

let mut decode_errors: usize = 0;
let decoded = loop {
let current_frame = probed.format.next_packet()?;
Expand Down Expand Up @@ -107,6 +118,7 @@ impl SymphoniaDecoder {
format: probed.format,
buffer,
spec,
duration,
}));
}

Expand Down Expand Up @@ -137,7 +149,7 @@ impl Source for SymphoniaDecoder {

#[inline]
fn total_duration(&self) -> Option<Duration> {
None
self.duration
}
}

Expand Down
Loading