Skip to content

Fix Symphonia seek for a duration with a fractional element of 0 #687

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.

Sign up for GitHub

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

Closed
will3942 wants to merge 4 commits into from
Closed

Fix Symphonia seek for a duration with a fractional element of 0 #687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98ee48b
Fix Symphonia seek for a duration with a fractional element of 0
will3942 Jan 23, 2025
29a0fd4
Add regression test
will3942 Jan 23, 2025
e990a08
Fix mp3 to be exactly 0ms
will3942 Jan 23, 2025
0f7df18
Move test to integration test
will3942 Jan 23, 2025
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
Binary file added assets/music-zero-ms.mp3
View file
Open in desktop
Binary file not shown.
5 changes: 4 additions & 1 deletion src/decoder/symphonia.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,13 @@ fn skip_back_a_tiny_bit(
}: Time,
) -> Time {
frac -= 0.0001;
Copy link
Collaborator

@PetrGlad PetrGlad Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybeif frac < 0.0001 { } else { } instead?

Next to the beginning of the Source saturating_sub will leave seconds unchanged, and the whole effect would be to actually seek forward almost a second.

Copy link
Member

@yara-blue yara-blue Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

its never called at the beginning of the source. This handles seeking beyond a source end.

Copy link
Member

@yara-blue yara-blue Jan 24, 2025

Choose a reason for hiding this comment

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

so this is good to merge right?

Copy link
Collaborator

@PetrGlad PetrGlad Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If it fixes the problem - yes, we can merge. Since it is already an improvement.
We can handle other cases later if necessary.
The source itself can be less than a second long. The check compares with < 1ms overlap, but tick-back is 0.1ms. what about overlaps in range 0.1...1.0 ms? I wonder if there are any symphonia functions to work with its timestamps or ways to convert those from/to Duration.

Copy link
Member

@yara-blue yara-blue Jan 24, 2025

Choose a reason for hiding this comment

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

The source itself can be less than a second long

ahh damn, I did not think of that. Thanks for clarifying. No I agree now, lets fix this properly only then merge. This will need a comment in the code explaining this case or a future refactor might throw the code out.

Copy link
Member

@yara-blue yara-blue Feb 4, 2025

Choose a reason for hiding this comment

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

I placed this comment in the code:

        let time = if seek_beyond_end {
            let time = self.total_duration.expect("if guarantees this is Some");
            skip_back_a_tiny_bit(time) // some decoders can only seek to just before the end
        } else {
            pos.as_secs_f64().into()
        };

As I remember symphonia does not seek if the time is beyond the length of the track. In rodio we have defined seek to saturate at the track length. If symphonia now saturates we can remove this.

Copy link
Member

@roderickvd roderickvd Feb 4, 2025

Choose a reason for hiding this comment

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

When you seek to or beyond the end, you get an UnexpectedEof which is anyway what you get when the stream ends.

Copy link
Member

@yara-blue yara-blue Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Though that is completely valid behavior for symphonia its not what we want in rodio (seeking beyond source should saturate as that is easiest for the end user) we do this instead. We also still want to return UnexpectedEof in case of other errors.

Though reading it now is still kinda fragile when audio has no total_duration. I'm strongly for leaving it as is for now. Since we have more then enough going on atm :)

edit: and the next release is already going to be gigantic. Do feel free to open an issue and present alternative behavior.

Copy link
Member

@roderickvd roderickvd Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean with saturating? I assume the behavior should be that the iterator returns None. Is that also what you mean?

I would make (and have made) the Symphonia decoder such that it handles this specific end-of-stream UnexpectedEof that way.

Copy link
Member

@yara-blue yara-blue Feb 4, 2025

Choose a reason for hiding this comment

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

What do you mean with saturating?

see the docs: https://docs.rs/rodio/latest/rodio/source/trait.Source.html#method.try_seek

As long as the duration of the source is known seek is guaranteed to saturate at the end of the source. For example given a source that reports a total duration of 42 seconds calling try_seek() with 60 seconds as argument will seek to 42 seconds.

I would make (and have made) the Symphonia decoder such that it handles this specific end-of-stream UnexpectedEof that way.

Yeah that's way simpler, go ahead and do that 👍 Maybe I was worried about skipping back? but that's not a feature nor can we make it one.


// Frac must be between 0.0 and 1.0 for Symphonia
if frac < 0.0 {
seconds = seconds.saturating_sub(1);
frac = 1.0 - frac;
frac += 1.0;
}

Time { seconds, frac }
}

Expand Down
12 changes: 12 additions & 0 deletions tests/seek.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,18 @@ fn seek_does_not_break_channel_order(
}
}

#[rstest]
fn seek_beyond_end_saturates_fixed_length_mp3() {
let asset = Path::new("assets/music-zero-ms.mp3");
let file = std::fs::File::open(asset).unwrap();
let mut decoder = rodio::Decoder::new(BufReader::new(file)).unwrap();

let res = decoder.try_seek(Duration::from_millis(195000));

assert!(res.is_ok(), "err: {res:?}");
assert!(time_remaining(decoder) < Duration::from_secs(1));
}

fn second_channel_beep_range<R: rodio::Source>(source: &mut R) -> std::ops::Range<usize>
where
R: Iterator<Item = f32>,
Expand Down
Loading