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

flac, isomp4, mkv: optimize length detection by using byte_len() instead of seeking #340

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

roderickvd
Copy link
Contributor

Replace seeking to end of file with byte_len() call to get total stream length. This improves performance when streaming files by avoiding an unnecessary seek operation. The change maintains the same functionality while being more efficient, especially for remote/streaming files where seeks are expensive.

As used in pleezer, previously seeking would block the application until the full file was downloaded, even if the position seemed to was already buffered. With this change, such seeks are nearly instantaneous.

Optionally, we could first try byte_len() and fall back to seeking to the end when byte_len() is not available.

The actual change is just three lines, but my editor automatically fixed some minor code formatting issues with else clauses. Nothing more than rustfmt.

…ead of seeking

Replace seeking to end of file with byte_len() call to get total stream
length. This improves performance when streaming files by avoiding an
unnecessary seek operation. The change maintains the same functionality
while being more efficient, especially for remote/streaming files where
seeks are expensive.

Also fixes some minor code formatting issues with else clauses.
roderickvd added a commit to roderickvd/pleezer that referenced this pull request Jan 16, 2025
Uses a patched version of Symphonia that improves seeking performance
in FLAC and MP4 files by using the known file byte length instead of
seeking to the end of file to determine length.

This is a temporary patch that will be removed once
pdeljanov/Symphonia#340 is merged upstream.
@pdeljanov
Copy link
Owner

pdeljanov commented Jan 18, 2025

Hi @roderickvd, yeah, that's the way it should've been. Nice find!

The reason you got so many formatting changes is because we need the nightly rustfmt to use the brace style option. Are you able to run that with cargo +nightly fmt?

By the way, please keep an eye on the dev-0.6 branch as that will be the future of Symphonia in a little while.

@roderickvd
Copy link
Contributor Author

Hi @roderickvd, yeah, that's the way it should've been. Nice find!

Cool.

Some questions after thinking over seeking some more:

  • Currently, when byte_len == None then seeking will bail out with a Unseekable error for all these formats. Looking at the MP3 implementation, that's true for coarse seeking, but not for accurate seeking. With accurate seeking in MP3 an attempt is made to rewind to the beginning and then do a linear search. Would this be a viable strategy for the other formats also?

  • For MP3, it's a hard-fail when SeekMode == Coarse and byte_len == None. I guess it could be changed to soft-fail by falling back to accurate (linear) seeking. Less obvious to the user, but perhaps more in line with the user intent to "please just seek"?

  • The other formats only implement accurate seeking, using a binary search. Would MP3's coarse preseeking be viable to implement for them too? I'm no audio format expert, just from a cursory code read imagine it would be possible.

  • Beyond ergonomics, note that Rodio wraps all sources in a ReadSeekSource struct with byte_len always set to None. So, in all transparency, this PR will break Rodio's seeking for everything but MP3. This is a shortcoming in Rodio that I intend to contribute to. In parallel I'm wondering if a "rewind-and-linear-search" fallback strategy could prevent such foot-shooting.

Can move to a discussion to not lead this PR astray too much.

The reason you got so many formatting changes is because we need the nightly rustfmt to use the brace style option. Are you able to run that with cargo +nightly fmt?

Sure, just did so in f143245. Let me know if you want me to squash.

By the way, please keep an eye on the dev-0.6 branch as that will be the future of Symphonia in a little while.

Yes, will do. You think this could make its way into a 0.5.5 release? For pleezer and librespot it would be great if I could keep depending on a "stable" release while dev-0.6 work is ongoing.

@roderickvd
Copy link
Contributor Author

  • Beyond ergonomics, note that Rodio wraps all sources in a ReadSeekSource struct with byte_len always set to None. So, in all transparency, this PR will break Rodio's seeking for everything but MP3. This is a shortcoming in Rodio that I intend to contribute to. In parallel I'm wondering if a "rewind-and-linear-search" fallback strategy could prevent such foot-shooting.

Refactoring Rodio's decoder to take a byte_len here: RustAudio/rodio#697

@pdeljanov
Copy link
Owner

Some questions after thinking over seeking some more:

  • Currently, when byte_len == None then seeking will bail out with a Unseekable error for all these formats. Looking at the MP3 implementation, that's true for coarse seeking, but not for accurate seeking. With accurate seeking in MP3 an attempt is made to rewind to the beginning and then do a linear search. Would this be a viable strategy for the other formats also?

There's no timing information in a MP3, so to seek in a sample accurate manner the only way is to count samples from the beginning of the file. In contrast, coarse seeking estimates the average bitrate of the file and then uses it to estimate where the reader should seek to. If the underlying source is unseekable, then we assume the seek function would just not work. So we can only seek forward from the current position by again consuming MPEG frames and counting.

Other formats are generally better in this regard. They either contain a table mapping timestamp-to-position, or each packet is timestamped so we can binary search. If I recall correctly, most of these readers will perform a seek in two stages: seek to a rough position guided by the table or binary search algorithm, then refine the position by reading sequentially until the desired timestamp is found. Usually, if the source is unseekable, it just falls back to the second stage so forward seeks work atleast. If this fallback is missing anywhere, then yes, we could do it!

  • For MP3, it's a hard-fail when SeekMode == Coarse and byte_len == None. I guess it could be changed to soft-fail by falling back to accurate (linear) seeking. Less obvious to the user, but perhaps more in line with the user intent to "please just seek"?

I believe this should work, but it may be failing if byte_len == None but is_seekable == true. There is an unstated assumption that byte_len != None if is_seekable == true because you could always seek to get the byte length. The documentation for implementers of the MediaSource trait could be improved here.

  • The other formats only implement accurate seeking, using a binary search. Would MP3's coarse preseeking be viable to implement for them too? I'm no audio format expert, just from a cursory code read imagine it would be possible.

Hmm, possibly. Though I guess this would be an optimistic optimization: no need to seek multiple times for a binary search if a first-try guess gets close enough.

  • Beyond ergonomics, note that Rodio wraps all sources in a ReadSeekSource struct with byte_len always set to None. So, in all transparency, this PR will break Rodio's seeking for everything but MP3. This is a shortcoming in Rodio that I intend to contribute to. In parallel I'm wondering if a "rewind-and-linear-search" fallback strategy could prevent such foot-shooting.

Noted!

Since I imagine most people use Symphonia through Rodio, this would be a pretty bad breakage. It would be great if Rodio could push an update with your fixes before this lands in a Symphonia release.

By the way, please keep an eye on the dev-0.6 branch as that will be the future of Symphonia in a little while.

Yes, will do. You think this could make its way into a 0.5.5 release? For pleezer and librespot it would be great if I could keep depending on a "stable" release while dev-0.6 work is ongoing.

Yeah, we could include this in 0.5.5. I was planning to cut a release before master becomes 0.6.

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.

2 participants