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

Make ogg muxer compliant with the ffmpeg demuxer (and the spec). #3062

Merged
merged 2 commits into from May 11, 2023

Conversation

toots
Copy link
Member

@toots toots commented May 6, 2023

There's been a couple of issues reported lately pointing to some problems with chained ogg bitstreams, most notably #2965, #3010, #2917 so I started investigating it.

The issues that we are tracking are:

  • The validity of chained ogg bitstreams (that is bitstreams with more than one track)

Are the stream we implement valid? Where is the spec? What are the other implementations saying?

  • Support for metadata in chained ogg bitstreams

Can other demuxers read metadata we produce on chained bitstreams? Can we read metadata on chained bitstreams?

This has raised some interesting issues that should be relevant for the icecast streaming community at large, now that liquidsdoap is used widely in this field.

⚠️ Invalid chained ogg bitstream ⚠️

Back in the days when we wrote our ogg muxer, and we're talking about 2009, some 14 years ago (see: cc5a44d), there wasn't a lot of clarity in how to synchronously end a logical ogg stream. That is, take an encoder at any point and ask it to end the stream and flush all remaining pages in it.

libvorbis provided an API for it but this was the only library being explicitly coupled with libogg. For all other libraries, the encoder might not have any data to flush when ending it and libogg required at least some data to produce the last page.

Thus, we went with the trick of submitting an empty ogg packet with the e_o_s flag set on it. Nothing in the spec prohibited it and it seems reasonable to expect that a decoder would simply consider an empty packet as the end of stream as is standard in unix.

Fast forward 14 years, two things happened:

  • The FFmpeg implementation for ogg demuxing considers an empty packet as invalid data.
  • The opus spec started using empty packets for packet loss control

This means that the 2016 rfc for ogg/opus encapsulation now prohibits empty packets: https://datatracker.ietf.org/doc/html/rfc7845#section-6. A better knowledge of opus codec from the 2012 codec spec could have pointed to this: https://datatracker.ietf.org/doc/html/rfc6716#section-3.4. This is the same time we did our own implementation of the ogg encapsulation (see: savonet/ocaml-opus@c0cd32b)

This means that our original assumption is no longer valid and that we are producing invalid ogg bitstreams at least from the perspective of FFmpeg (for all non-vorbis codecs) and all fully compliant opus decoders.

To fix that, a semi hacked Ogg.Stream.terminate was implemented in savonet/ocaml-ogg@78edbb2 to allow the production of a final, empty EOS page when needed, something that, this time, is actually explicitly allowed by the spec: https://datatracker.ietf.org/doc/html/rfc3533#section-4

Eos pages may be 'nil' pages, that is, pages
containing no content but simply a page header with position
information and the eos flag set in the page header.

This PR adds support for this new function to all ogg codecs that previously relied on pushing an empty eos packet. This applies to speex, theora and opus.

For flac, this PR and savonet/ocaml-flac@10aecfc switch to libflac native ogg encapsulation implementation, which also makes us compliant with their spec and future-proof. This should fix #2965

Chained metadata

The other side of this problem is that, for too long, we were testing ogg streams in a closed off environment, that is, decoding streams produced by our own encoder. But now that input.http is using ffmpeg internally, it became clear that something was off with ogg chained bitstream metadata.

After fixing our internal bitstream, FFmpeg started being able to parse all metadata in ogg/opus streams, albeit with a memory leak as the library forgets to clear previous metadata when adding new, which results in the new metadata being appended. This PR implements a workaround for that and a patch will be submitted to FFmpeg soon.

However, ogg/flac streams are still missing chained stream metadata. This is due to a different ways that FFmpeg parses chained ogg/flac bitstream. A patch has already been submitted and will hopefully be applied at some point.

@toots toots requested a review from smimram May 6, 2023 23:35
@toots toots mentioned this pull request May 7, 2023
@toots toots force-pushed the ffmpeg-compliant-ogg-muxer branch from 214eeed to d2b7b30 Compare May 9, 2023 23:00
@toots toots force-pushed the ffmpeg-compliant-ogg-muxer branch from d2b7b30 to 56fedee Compare May 11, 2023 01:33
@toots toots merged commit 02da14b into main May 11, 2023
19 checks passed
@toots toots deleted the ffmpeg-compliant-ogg-muxer branch May 11, 2023 06:05
@gabsoftware
Copy link

Aw bummer, the CI failed for that one because of some apt update fun, so it's not in the 2.2x rolling release yet.

@toots
Copy link
Member Author

toots commented May 11, 2023

Should be good now!

@gabsoftware
Copy link

Yay, thanks! Will test ASAP! 👍

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.

Icecast ogg/flac output keeps disconnecting when using SRT input [2.2.0 nightly]
2 participants