Potential Regression: `try_seek` Panicking When Seeking Backwards

[UnknownSuperficialNight]

Switching from rodio 0.20.1 to the main branch (with symphonia-all enabled), I encountered an issue where try_seek produces a SymphoniaDecoder(BaseSeek(SeekError(ForwardOnly))) error when attempting to seek backwards. This behavior was not present in rodio 0.20.1. I've tested with multiple file formats (flac, m4a, and mp3), all of which produce the same error when seeking to a previous time.

Additional Testing: When using music.ogg from the assets folder, the try_seek function does not seek to the specified position. Instead, it appears to seek to a slightly earlier position and then continues playback as if the seek was relative.

Reproduction Build

rodio_seek_err.zip

[roderickvd]

Switching from rodio 0.20.1 to the main branch (with symphonia-all enabled), I encountered an issue where try_seek produces a SymphoniaDecoder(BaseSeek(SeekError(ForwardOnly))) error when attempting to seek backwards. This behavior was not present in rodio 0.20.1. I've tested with multiple file formats (flac, m4a, and mp3), all of which produce the same error when seeking to a previous time.

Change Decoder::new(BufReader::new(file)) to Decoder::try_from(file) and you'll see it works as expected.

This is due to the fact that in v0.20, Symphonia assumed all sources were seekable. v0.21 changes that assumption, but does the right thing to auto-detect it when you try to convert from a File or BufReader. You can also use the new DecoderBuilder::with_seekable.

@dvdsk we should probably add this to the upgrading guide.

Additional Testing: When using music.ogg from the assets folder, the try_seek function does not seek to the specified position. Instead, it appears to seek to a slightly earlier position and then continues playback as if the seek was relative.

rodio_seek_err.zip

Thanks for providing this minimal example.

This is a Symphonia bug. For many demuxers, Symphonia implemented returning a SeekError::ForwardOnly, but not for Ogg. So with your minimal example we're actually getting stuff like:

SeekedTo {
    track_id: 369351377,
    required_ts: 176400,
    actual_ts: 512960,
}

We could put a band-aid on and throw a ForwardOnly error manually when actual_ts > required_ts but I'd rather have it fixed upstream.

[dvdsk]

@dvdsk we should probably add this to the upgrading guide.

will do as soon as Rustweek (conference) is over. So weekend, then we can probably also release after right?

[dvdsk]

Reproduction Build

rodio_seek_err.zip

I just want to also call out the minimal example you provided. Thank you very much for that and for reporting the issue 🤟.

[UnknownSuperficialNight]

Change Decoder::new(BufReader::new(file)) to Decoder::try_from(file) and you'll see it works as expected.

This is due to the fact that in v0.20, Symphonia assumed all sources were seekable. v0.21 changes that assumption, but does the right thing to auto-detect it when you try to convert from a File or BufReader. You can also use the new DecoderBuilder::with_seekable.

Thank you for the example.

Using the DecoderBuilder::with_seekable does solve it.

@dvdsk we should probably add this to the upgrading guide.

I think it might potentially be worth it to maybe also add a small note/mention in the docs for Decoder::new that if the user wants seekable audio they should see DecoderBuilder::with_seekable or Decoder::try_from

I'll leave this issue open as a reminder for @dvdsk feel free to close it whenever

[dvdsk]

@roderickvd what would happen if we enable seeking by default on every symphonia decoder? I rather have runtime errors on unseekable media then a runtime error on seekable things because seeking has not been enabled.

I feel like I am missing the reason why we have the seekable option.

[roderickvd]

I was also thinking to default to enabled, but have been undecided. It’s about what’s the lesser of evils.

The reason is that Symfonia requires byte_len to be set when is_seekable is true. Without it, it indeed will return an error that seeking is not supported.

Just to throw it out there, there’s another option we could implement in Decoder::new: seeking the file to the end to determine the byte length. This would fail on infinite streams, and succeed otherwise.

Given that streams are probably the exceptional case, that may be the better option? And have users use the builder for streams.

[dvdsk]

The reason is that Symfonia requires byte_len to be set when is_seekable is true. Without it, it indeed will return an error that seeking is not supported.

assuming that it returns that error when seeking only. Can we just pass that on with a suggestion to enable seeking?

Just to throw it out there, there’s another option we could implement in Decoder::new: seeking the file to the end to determine the byte length. This would fail on infinite streams, and succeed otherwise.

Would probably result in (to the user) unexplained hanging here and there. Like in a (sup-optimally) designed audio app, you do not want adding to a queue to hang the UI for a second.

[roderickvd]

The reason is that Symfonia requires byte_len to be set when is_seekable is true. Without it, it indeed will return an error that seeking is not supported.

assuming that it returns that error when seeking only. Can we just pass that on with a suggestion to enable seeking?

I’m understanding you suggest to enable is_seekable by default regardless of whether byte_len is set, then decorate the SeekError::NotSupported with a message to ensure byte_len is set. That is also an option.

The byte_len is part of the Mediasource which is moved into the Symphonia decoder. So we don’t have access to the field anymore. But we could make the help even more helpful by checking the value in Decoder::new and tuning the error message to it.

[dvdsk]

I’m understanding you suggest to enable is_seekable by default regardless of whether byte_len is set, then decorate the SeekError::NotSupported with a message to ensure byte_len is set. That is also an option.

To me that seems the easiest for a beginner. Given rodio has a lot of those I would say lets optimize for that experience.

The byte_len is part of the Mediasource which is moved into the Symphonia decoder. So we don’t have access to the field anymore. But we could make the help even more helpful by checking the value in Decoder::new and tuning the error message to it.

That is a great idea, lets do that!

[dvdsk]

The byte_len is part of the Mediasource which is moved into the Symphonia decoder. So we don’t have access to the field anymore. But we could make the help even more helpful by checking the value in Decoder::new and tuning the error message to it.

I'll pick this up, I think its the last thing we need before the next release

[dvdsk]

okay its not simple: pdeljanov/Symphonia#340 (comment)

[roderickvd]

I'll pick this up, I think its the last thing we need before the next release

Sorry, was preoccupied with another project.

okay its not simple: pdeljanov/Symphonia#340 (comment)

I don't know, can't we make it simple?

• I was thinking we could just inspect mss.byte_len() in the Symphonia decoder constructor:

  rodio/src/decoder/symphonia.rs

  Line 34 in d2313b0

  pub(crate) fn new(mss: MediaSourceStream, settings: &Settings) -> Result<Self, DecoderError> {

  
  and then cache it in the a struct field, which you could use in try_seek.

• Your point over at Symphonia, that some codecs require it and some don't, is a bug in Symphonia. That PR over there addresses exactly that. That some codecs work without it, is because Symphonia then seeks to the end. We discussed that, and we don't want that to happen automatically.

Unless I'm missing something, with that, it should be pretty simple to do.

[dvdsk]

relevant PR: #744, still problematic: can not open m4a without setting seekable to true (even if we have no bytes_len).