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

Ogg chaining support #261

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Ogg chaining support #261

wants to merge 5 commits into from

Conversation

ziplantil
Copy link

@ziplantil ziplantil commented Oct 30, 2021

Related: #254

This commit, possibly set of commits in the future, aims to add support for Ogg chaining (concatenation) and handling chained streams when decoding Ogg FLAC streams. Before these changes, attempts to decode chained streams could result in one of two things:

  • If the Ogg stream serial numbers are the same for the previous and next stream, the decoder would think it went out of sync
  • If they are different, the decoder will simply ignore any pages belonging to the latter; on streams with no definite end, such as online streams, this would cause the decoder to read forever

Two new API calls are added (due to this I envision having to update the version number): FLAC__stream_decoder_set_ogg_allow_chaining and FLAC__stream_decoder_get_ogg_allow_chaining (and the C++ equivalents using the same naming convention as existing calls). The new feature is only enabled if the user explicitly requests it; this preserves backwards compatibility. Specifically, a user must do FLAC__stream_decoder_set_ogg_allow_chaining(decoder, true) to enable this functionality.

When enabled, the Ogg aspect decoder will try to check if the end-of-stream flag is set for a page. If this is the case, a new Ogg flag is set that tells the FLAC decoder to reset itself. It will then proceed to do so; the reset is only partial and will simply cause the FLAC decoder to assume that we begun a new decode process. This will cause metadata, including STREAMINFO, to be re-read.

The user could detect stream changes by listening to STREAMINFO metadata updates.

The intended use case of supporting Ogg chaining is, among others, to support playback of Ogg FLAC online radio streams that use chaining similar to how Ogg Vorbis streams would be implemented.

These changes do not have tests right now, since I am not sure what the best approach would be to test a feature like this. Likewise, the new feature exists only in libFLAC(++) for now; the flac program currently does not use this functionality.

@ziplantil
Copy link
Author

Draftifying for now due to potential issues such as seeking, as discussed in #254

@ziplantil ziplantil marked this pull request as draft November 4, 2021 12:08
@ktmf01
Copy link
Collaborator

ktmf01 commented Nov 11, 2021

Perhaps we can take a look at libopusfile for inspiration about seeking? Seeing these lines of code, it seems they've already figured it out: https://github.com/xiph/opusfile/blob/b23e611ffbed22886dfc1640abfcfd5bc6677b72/src/opusfile.c#L31-L59

@ktmf01 ktmf01 mentioned this pull request Apr 15, 2022
@ktmf01
Copy link
Collaborator

ktmf01 commented Nov 21, 2022

I've been pondering this issue for a while. I've come up with a few decisions that need to be made

  • What to do with metadata. After all, that is one reason chained ogg is used. If we say the decoder emits metadata for each link, the decoder must be aware of this possibility, and the 'allow chaining' functions are really necessary. At first I thought it would be better for libFLAC to support this transparently, but this will break users as they do not expect metadata after the first audio frame.
  • It might also be possible to decode all audio for all users, but only emit new metadata when enabled by the user. However, this might be weird as all audio will be played but part of the metadata is hidden.
  • Should libFLAC emit metadata after a seek crossing a chain link? That way the user of libFLAC can update displayed metadata. Or should the user be expected to process the metadata of all links before starting playback? That could result in large memory usage, especially when album art is involved.

@philippe44
Copy link

I gave that another try here #667. I think that this PR does not work for the reason I mention at the beginning of my PR: there is no direct control between pulling the last page from the Ogg stream and when that page will reach the Flac decoder. I might be wrong, but otherwise, although it does not crash, you lose Ogg synchronization and miss some last flac frames. I've tested my PR with hours of ogf webradio.

For the seeking question, it candidly seems to me like a lot of work for now with a dubious use case. Obviously the decision is to the maintainers, but I'd rather have a first step with a disclaimer that seeking is not supported in chained files, knowing that the most important use case if live streams where seeking does not make sense.

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.

None yet

3 participants