[PATCH v2] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat memory storage for the extradata. PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout --- libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++- tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 - 3 files changed, 114 insertions(+), 14 deletions(-) diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c index adbd726183..84879462a1 100644 --- a/libavcodec/vorbisdec.c +++ b/libavcodec/vorbisdec.c @@ -1778,11 +1778,40 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame, GetBitContext *gb = &vc->gb; float *channel_ptrs[255]; int i, len, ret; + size_t new_extradata_size; + const uint8_t *new_extradata; + const uint8_t *header_start[3]; + int header_len[3] = {0, 0, 0}; + const uint8_t *header; + int header_size = 0; + const uint8_t *comment; + int comment_size = 0; + const uint8_t *setup; + int setup_size = 0; ff_dlog(NULL, "packet length %d \n", buf_size); - if (*buf == 1 && buf_size > 7) { - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0) + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, + &new_extradata_size); + + if (new_extradata) { + ret = avpriv_split_xiph_headers(new_extradata, new_extradata_size, + 30, header_start, header_len); + if (ret < 0) + return ret; + + header = header_start[0]; + header_size = header_len[0]; + + comment = header_start[1]; + comment_size = header_len[1]; + + setup = header_start[2]; + setup_size = header_len[2]; + } + + if (header_size > 7 && *header == 1) { + if ((ret = init_get_bits8(gb, header + 1, header_size - 1)) < 0) return ret; vorbis_free(vc); @@ -1801,16 +1830,14 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame, } avctx->sample_rate = vc->audio_samplerate; - return buf_size; } - if (*buf == 3 && buf_size > 7) { + if (comment_size > 7 && *comment == 3) { av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n"); - return buf_size; } - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) { - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0) + if (setup_size > 7 && *setup == 5 && vc->channel_residues && !vc->modes) { + if ((ret = init_get_bits8(gb, setup + 1, setup_size - 1)) < 0) return ret; if ((ret = vorbis_parse_setup_hdr(vc))) { @@ -1818,7 +1845,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame, vorbis_free(vc); return ret; } - return buf_size; } if (!vc->channel_residues || !vc->modes) { diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c index 62cc2da6de..7859ec5a51 100644 --- a/libavformat/oggparsevorbis.c +++ b/libavformat/oggparsevorbis.c @@ -215,6 +215,12 @@ struct oggvorbis_private { AVVorbisParseContext *vp; int64_t final_pts; int final_duration; + uint8_t *header; + int header_size; + uint8_t *comment; + int comment_size; + uint8_t *setup; + int setup_size; }; static int fixup_vorbis_headers(AVFormatContext *as, @@ -260,6 +266,10 @@ static void vorbis_cleanup(AVFormatContext *s, int idx) av_vorbis_parse_free(&priv->vp); for (i = 0; i < 3; i++) av_freep(&priv->packet[i]); + + av_freep(&priv->header); + av_freep(&priv->comment); + av_freep(&priv->setup); } } @@ -434,6 +444,9 @@ static int vorbis_packet(AVFormatContext *s, int idx) struct ogg_stream *os = ogg->streams + idx; struct oggvorbis_private *priv = os->private; int duration, flags = 0; + int skip_packet = 0; + int ret, new_extradata_size; + PutByteContext pb; if (!priv->vp) return AVERROR_INVALIDDATA; @@ -496,10 +509,50 @@ static int vorbis_packet(AVFormatContext *s, int idx) if (duration < 0) { os->pflags |= AV_PKT_FLAG_CORRUPT; return 0; - } else if (flags & VORBIS_FLAG_COMMENT) { - vorbis_update_metadata(s, idx); + } + + if (flags & VORBIS_FLAG_HEADER) { + ret = vorbis_parse_header(s, s->streams[idx], os->buf + os->pstart, os->psize); + if (ret < 0) + return ret; + + ret = av_reallocp(&priv->header, os->psize); + if (ret < 0) + return ret; + + memcpy(priv->header, os->buf + os->pstart, os->psize); + priv->header_size = os->psize; + + skip_packet = 1; + } + + if (flags & VORBIS_FLAG_COMMENT) { + ret = vorbis_update_metadata(s, idx); + if (ret < 0) + return ret; + + ret = av_reallocp(&priv->comment, os->psize); + if (ret < 0) + return ret; + + memcpy(priv->comment, os->buf + os->pstart, os->psize); + priv->comment_size = os->psize; + flags = 0; + skip_packet = 1; + } + + if (flags & VORBIS_FLAG_SETUP) { + ret = av_reallocp(&priv->setup, os->psize); + if (ret < 0) + return ret; + + memcpy(priv->setup, os->buf + os->pstart, os->psize); + priv->setup_size = os->psize; + + skip_packet = 1; } + os->pduration = duration; } @@ -521,7 +574,31 @@ static int vorbis_packet(AVFormatContext *s, int idx) priv->final_duration += os->pduration; } - return 0; + if (priv->header && priv->comment && priv->setup) { + new_extradata_size = priv->header_size + priv->comment_size + priv->setup_size + 6; + + ret = av_reallocp(&os->new_extradata, new_extradata_size); + if (ret < 0) + return ret; + + os->new_extradata_size = new_extradata_size; + bytestream2_init_writer(&pb, os->new_extradata, new_extradata_size); + bytestream2_put_be16(&pb, priv->header_size); + bytestream2_put_buffer(&pb, priv->header, priv->header_size); + bytestream2_put_be16(&pb, priv->comment_size); + bytestream2_put_buffer(&pb, priv->comment, priv->comment_size); + bytestream2_put_be16(&pb, priv->setup_size); + bytestream2_put_buffer(&pb, priv->setup, priv->setup_size); + + av_freep(&priv->header); + priv->header_size = 0; + av_freep(&priv->comment); + priv->comment_size = 0; + av_freep(&priv->setup); + priv->comment_size = 0; + } + + return skip_packet; } const struct ogg_codec ff_vorbis_codec = { diff --git a/tests/ref/fate/ogg-vorbis-chained-meta.txt b/tests/ref/fate/ogg-vorbis-chained-meta.txt index b7a97c90e2..1206f86c1f 100644 --- a/tests/ref/fate/ogg-vorbis-chained-meta.txt +++ b/tests/ref/fate/ogg-vorbis-chained-meta.txt @@ -6,10 +6,7 @@ Stream ID: 0, frame PTS: 128, metadata: N/A Stream ID: 0, packet PTS: 704, packet DTS: 704 Stream ID: 0, frame PTS: 704, metadata: N/A Stream ID: 0, packet PTS: 0, packet DTS: 0 -Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, new metadata: encoder=Lavc61.19.100 libvorbis:title=Second Stream -Stream ID: 0, packet PTS: 0, packet DTS: 0 -Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, frame PTS: 0, metadata: N/A Stream ID: 0, packet PTS: 128, packet DTS: 128 Stream ID: 0, frame PTS: 128, metadata: N/A -- 2.39.5 (Apple Git-154)
Hi all, Le mer. 4 juin 2025 à 11:58, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
Is there any interest in reviewing this patch? It's holding the second series that fixes chained ogg stream metadata parsing. Better support for chained ogg streams would really make life easier for a bunch of ffmpeg and ffmpeg libraries users. Thanks, -- Romain
--- libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++- tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 - 3 files changed, 114 insertions(+), 14 deletions(-)
diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c index adbd726183..84879462a1 100644 --- a/libavcodec/vorbisdec.c +++ b/libavcodec/vorbisdec.c @@ -1778,11 +1778,40 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame, GetBitContext *gb = &vc->gb; float *channel_ptrs[255]; int i, len, ret; + size_t new_extradata_size; + const uint8_t *new_extradata; + const uint8_t *header_start[3]; + int header_len[3] = {0, 0, 0}; + const uint8_t *header; + int header_size = 0; + const uint8_t *comment; + int comment_size = 0; + const uint8_t *setup; + int setup_size = 0;
ff_dlog(NULL, "packet length %d \n", buf_size);
- if (*buf == 1 && buf_size > 7) { - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0) + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, + &new_extradata_size); + + if (new_extradata) { + ret = avpriv_split_xiph_headers(new_extradata, new_extradata_size, + 30, header_start, header_len); + if (ret < 0) + return ret; + + header = header_start[0]; + header_size = header_len[0]; + + comment = header_start[1]; + comment_size = header_len[1]; + + setup = header_start[2]; + setup_size = header_len[2]; + } + + if (header_size > 7 && *header == 1) { + if ((ret = init_get_bits8(gb, header + 1, header_size - 1)) < 0) return ret;
vorbis_free(vc); @@ -1801,16 +1830,14 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame, }
avctx->sample_rate = vc->audio_samplerate; - return buf_size; }
- if (*buf == 3 && buf_size > 7) { + if (comment_size > 7 && *comment == 3) { av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n"); - return buf_size; }
- if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) { - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0) + if (setup_size > 7 && *setup == 5 && vc->channel_residues && !vc->modes) { + if ((ret = init_get_bits8(gb, setup + 1, setup_size - 1)) < 0) return ret;
if ((ret = vorbis_parse_setup_hdr(vc))) { @@ -1818,7 +1845,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame, vorbis_free(vc); return ret; } - return buf_size; }
if (!vc->channel_residues || !vc->modes) { diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c index 62cc2da6de..7859ec5a51 100644 --- a/libavformat/oggparsevorbis.c +++ b/libavformat/oggparsevorbis.c @@ -215,6 +215,12 @@ struct oggvorbis_private { AVVorbisParseContext *vp; int64_t final_pts; int final_duration; + uint8_t *header; + int header_size; + uint8_t *comment; + int comment_size; + uint8_t *setup; + int setup_size; };
static int fixup_vorbis_headers(AVFormatContext *as, @@ -260,6 +266,10 @@ static void vorbis_cleanup(AVFormatContext *s, int idx) av_vorbis_parse_free(&priv->vp); for (i = 0; i < 3; i++) av_freep(&priv->packet[i]); + + av_freep(&priv->header); + av_freep(&priv->comment); + av_freep(&priv->setup); } }
@@ -434,6 +444,9 @@ static int vorbis_packet(AVFormatContext *s, int idx) struct ogg_stream *os = ogg->streams + idx; struct oggvorbis_private *priv = os->private; int duration, flags = 0; + int skip_packet = 0; + int ret, new_extradata_size; + PutByteContext pb;
if (!priv->vp) return AVERROR_INVALIDDATA; @@ -496,10 +509,50 @@ static int vorbis_packet(AVFormatContext *s, int idx) if (duration < 0) { os->pflags |= AV_PKT_FLAG_CORRUPT; return 0; - } else if (flags & VORBIS_FLAG_COMMENT) { - vorbis_update_metadata(s, idx); + } + + if (flags & VORBIS_FLAG_HEADER) { + ret = vorbis_parse_header(s, s->streams[idx], os->buf + os->pstart, os->psize); + if (ret < 0) + return ret; + + ret = av_reallocp(&priv->header, os->psize); + if (ret < 0) + return ret; + + memcpy(priv->header, os->buf + os->pstart, os->psize); + priv->header_size = os->psize; + + skip_packet = 1; + } + + if (flags & VORBIS_FLAG_COMMENT) { + ret = vorbis_update_metadata(s, idx); + if (ret < 0) + return ret; + + ret = av_reallocp(&priv->comment, os->psize); + if (ret < 0) + return ret; + + memcpy(priv->comment, os->buf + os->pstart, os->psize); + priv->comment_size = os->psize; + flags = 0; + skip_packet = 1; + } + + if (flags & VORBIS_FLAG_SETUP) { + ret = av_reallocp(&priv->setup, os->psize); + if (ret < 0) + return ret; + + memcpy(priv->setup, os->buf + os->pstart, os->psize); + priv->setup_size = os->psize; + + skip_packet = 1; } + os->pduration = duration; }
@@ -521,7 +574,31 @@ static int vorbis_packet(AVFormatContext *s, int idx) priv->final_duration += os->pduration; }
- return 0; + if (priv->header && priv->comment && priv->setup) { + new_extradata_size = priv->header_size + priv->comment_size + priv->setup_size + 6; + + ret = av_reallocp(&os->new_extradata, new_extradata_size); + if (ret < 0) + return ret; + + os->new_extradata_size = new_extradata_size; + bytestream2_init_writer(&pb, os->new_extradata, new_extradata_size); + bytestream2_put_be16(&pb, priv->header_size); + bytestream2_put_buffer(&pb, priv->header, priv->header_size); + bytestream2_put_be16(&pb, priv->comment_size); + bytestream2_put_buffer(&pb, priv->comment, priv->comment_size); + bytestream2_put_be16(&pb, priv->setup_size); + bytestream2_put_buffer(&pb, priv->setup, priv->setup_size); + + av_freep(&priv->header); + priv->header_size = 0; + av_freep(&priv->comment); + priv->comment_size = 0; + av_freep(&priv->setup); + priv->comment_size = 0; + } + + return skip_packet; }
const struct ogg_codec ff_vorbis_codec = { diff --git a/tests/ref/fate/ogg-vorbis-chained-meta.txt b/tests/ref/fate/ogg-vorbis-chained-meta.txt index b7a97c90e2..1206f86c1f 100644 --- a/tests/ref/fate/ogg-vorbis-chained-meta.txt +++ b/tests/ref/fate/ogg-vorbis-chained-meta.txt @@ -6,10 +6,7 @@ Stream ID: 0, frame PTS: 128, metadata: N/A Stream ID: 0, packet PTS: 704, packet DTS: 704 Stream ID: 0, frame PTS: 704, metadata: N/A Stream ID: 0, packet PTS: 0, packet DTS: 0 -Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, new metadata: encoder=Lavc61.19.100 libvorbis:title=Second Stream -Stream ID: 0, packet PTS: 0, packet DTS: 0 -Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, frame PTS: 0, metadata: N/A Stream ID: 0, packet PTS: 128, packet DTS: 128 Stream ID: 0, frame PTS: 128, metadata: N/A -- 2.39.5 (Apple Git-154)
Hi Romain On Tue, Jun 10, 2025 at 01:04:35PM -0500, Romain Beauxis wrote:
Hi all,
Le mer. 4 juin 2025 à 11:58, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
Is there any interest in reviewing this patch?
maybe you can help review other peoples patches while you wait for someone to review yours (if everyone does that then reviews should overall occur quicker) (i do have a backlog of things atm so i dont think i should add this one to my todo) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data
Le jeu. 12 juin 2025 à 13:35, Michael Niedermayer <michael@niedermayer.cc> a écrit :
Hi Romain
Hi,
On Tue, Jun 10, 2025 at 01:04:35PM -0500, Romain Beauxis wrote:
Hi all,
Le mer. 4 juin 2025 à 11:58, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a
flat
memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
Is there any interest in reviewing this patch?
maybe you can help review other peoples patches while you wait for someone to review yours (if everyone does that then reviews should overall occur quicker) (i do have a backlog of things atm so i dont think i should add this one to my todo)
I do appreciate the invite to contribute to reviews and will gladly do so. I understand how the project needs to balance load and how it's nice to contribute both ways. However, I'm still learning the ropes of this code base (and this patch proves it). It seems awkward to make this transactional, there's a discrepancy in what I can provide in terms of senior review and this patch is rather small and implements exactly what I was asked to do after multiple consultations. I was hoping it would be easy to review and merge. Thanks, -- Romain
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote:
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
---
libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable have not reveiwed the rest of the patch thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once" - "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote:
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
---
libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect
if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
have not reveiwed the rest of the patch
thx
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 1 "Used only once" - "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..." _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, Jun 21, 2025 at 10:45:32AM +0200, Romain Beauxis wrote:
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote:
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
---
libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect
if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
If there is really no API extension then the micro versions should be bumped so a user knows if the specific version he uses has teh fix. Also it may be usefull in bugreports about ogg to know if its prior or after this change thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
Le sam. 21 juin 2025 à 16:59, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Sat, Jun 21, 2025 at 10:45:32AM +0200, Romain Beauxis wrote:
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote:
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a
flat
memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
---
libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect
if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
If there is really no API extension then the micro versions should be bumped so a user knows if the specific version he uses has teh fix. Also it may be usefull in bugreports about ogg to know if its prior or after this change
Hi Michael, I have created a PR on code.ffmpeg.org here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026 Would you have a minute to have a look? This patch is a redo of a patch that was reverted: https://code.ffmpeg.org/FFmpeg/FFmpeg/commit/848ceb1329cb6102df49379430b277d... It would be great to see if it could be included in the next release, otherwise the round of work on ogg parsing would be incomplete for it. I'm hoping that the PR can help gather and keep review feedback in one place. Thanks, -- Romain
thx
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hi Romain On Wed, Jul 23, 2025 at 02:06:07PM -0500, Romain Beauxis wrote:
Le sam. 21 juin 2025 à 16:59, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Sat, Jun 21, 2025 at 10:45:32AM +0200, Romain Beauxis wrote:
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote:
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a
flat
memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
---
libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect
if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
If there is really no API extension then the micro versions should be bumped so a user knows if the specific version he uses has teh fix. Also it may be usefull in bugreports about ogg to know if its prior or after this change
Hi Michael,
I have created a PR on code.ffmpeg.org here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026
Would you have a minute to have a look?
I do not have time, ive made a dumb mistake in configuring my inbox so iam about a week behind with emails without realizing it. thats on top of release, security, and other things maybe someome else can look into this one replying here so noone waits for a review from me thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Any man who breaks a law that conscience tells him is unjust and willingly accepts the penalty by staying in jail in order to arouse the conscience of the community on the injustice of the law is at that moment expressing the very highest respect for law. - Martin Luther King Jr
Le dim. 27 juil. 2025 à 19:22, Michael Niedermayer <michael@niedermayer.cc> a écrit :
Hi Romain
On Wed, Jul 23, 2025 at 02:06:07PM -0500, Romain Beauxis wrote:
Le sam. 21 juin 2025 à 16:59, Michael Niedermayer <
michael@niedermayer.cc>
a écrit :
On Sat, Jun 21, 2025 at 10:45:32AM +0200, Romain Beauxis wrote:
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote:
This is a redo of 574f634e49847e2225ee50013afebf0de03ef013
using a flat
memory storage for the extradata.
PR review comments addressed: * Use flat memory bytestream * Re-use existing xiph extradata layout
---
libavcodec/vorbisdec.c | 42 ++++++++--- libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect
if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
If there is really no API extension then the micro versions should be bumped so a user knows if the specific version he uses has teh fix. Also it may be usefull in bugreports about ogg to know if its prior or after this change
Hi Michael,
I have created a PR on code.ffmpeg.org here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026
Would you have a minute to have a look?
I do not have time, ive made a dumb mistake in configuring my inbox so iam about a week behind with emails without realizing it. thats on top of release, security, and other things
maybe someome else can look into this one replying here so noone waits for a review from me
Thanks for letting me know and sorry about your issues with your email inbox. Do you have any advice on how to look for a reviewer for this path?
thx
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Any man who breaks a law that conscience tells him is unjust and willingly accepts the penalty by staying in jail in order to arouse the conscience of the community on the injustice of the law is at that moment expressing the very highest respect for law. - Martin Luther King Jr _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hi On Mon, Jul 28, 2025 at 04:12:13PM -0500, Romain Beauxis wrote:
Le dim. 27 juil. 2025 à 19:22, Michael Niedermayer <michael@niedermayer.cc> a écrit :
Hi Romain
On Wed, Jul 23, 2025 at 02:06:07PM -0500, Romain Beauxis wrote:
Le sam. 21 juin 2025 à 16:59, Michael Niedermayer <
michael@niedermayer.cc>
a écrit :
On Sat, Jun 21, 2025 at 10:45:32AM +0200, Romain Beauxis wrote:
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit :
On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote: > This is a redo of 574f634e49847e2225ee50013afebf0de03ef013
using a flat
> memory storage for the extradata. > > PR review comments addressed: > * Use flat memory bytestream > * Re-use existing xiph extradata layout > > ---
> libavcodec/vorbisdec.c | 42 ++++++++--- > libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
patches that change both libraries at the same time are suspect
if one depends on changes in the other it needs minor API version bump and seperate patches so extension of API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
If there is really no API extension then the micro versions should be bumped so a user knows if the specific version he uses has teh fix. Also it may be usefull in bugreports about ogg to know if its prior or after this change
Hi Michael,
I have created a PR on code.ffmpeg.org here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026
Would you have a minute to have a look?
I do not have time, ive made a dumb mistake in configuring my inbox so iam about a week behind with emails without realizing it. thats on top of release, security, and other things
maybe someome else can look into this one replying here so noone waits for a review from me
Thanks for letting me know and sorry about your issues with your email inbox.
Do you have any advice on how to look for a reviewer for this path?
social media, like twitter also if it has a positive vibe then we can retweet thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
Le dim. 3 août 2025 à 16:36, Michael Niedermayer <michael@niedermayer.cc> a écrit :
Hi
On Mon, Jul 28, 2025 at 04:12:13PM -0500, Romain Beauxis wrote:
Le dim. 27 juil. 2025 à 19:22, Michael Niedermayer <michael@niedermayer.cc> a écrit :
Hi Romain
On Wed, Jul 23, 2025 at 02:06:07PM -0500, Romain Beauxis wrote:
Le sam. 21 juin 2025 à 16:59, Michael Niedermayer <
michael@niedermayer.cc>
a écrit :
On Sat, Jun 21, 2025 at 10:45:32AM +0200, Romain Beauxis wrote:
Le dim. 15 juin 2025 à 00:57, Michael Niedermayer <michael@niedermayer.cc> a écrit : > > On Wed, Jun 04, 2025 at 11:58:52AM -0500, Romain Beauxis wrote: > > This is a redo of 574f634e49847e2225ee50013afebf0de03ef013
using a flat
> > memory storage for the extradata. > > > > PR review comments addressed: > > * Use flat memory bytestream > > * Re-use existing xiph extradata layout > > > > --- > > > libavcodec/vorbisdec.c | 42 ++++++++--- > > libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++- > > patches that change both libraries at the same time are suspect > > if one depends on changes in the other it needs > minor API version bump and seperate patches so extension of > API and use of it are properly tracked and testable
If I remember well, according to Andreas Rheinhardt there's no need for an API bump here since the patch is re-using existing extradata bitstream structures.
If there is really no API extension then the micro versions should be bumped so a user knows if the specific version he uses has teh fix. Also it may be usefull in bugreports about ogg to know if its prior or after this change
Hi Michael,
I have created a PR on code.ffmpeg.org here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026
Would you have a minute to have a look?
I do not have time, ive made a dumb mistake in configuring my inbox so iam about a week behind with emails without realizing it. thats on top of release, security, and other things
maybe someome else can look into this one replying here so noone waits for a review from me
Thanks for letting me know and sorry about your issues with your email inbox.
Do you have any advice on how to look for a reviewer for this path?
social media, like twitter
also if it has a positive vibe then we can retweet
Respectfully, this does not make sense to me. I already spend way too much time on social media promoting things that actually need promotion like music shows. I do not want to have my contributions to a technical project be judged by the hype. I strive to do good work that has merit on its own. But, specifically and in relation to this patch, this also does not make sense because this patch is a technical fix from a long windy string of events. Let me recap: I initially submitted a one-liner patch to fix chained opus streams: https://ffmpeg.org/pipermail/ffmpeg-devel/2025-January/338538.html (This was Jan. 18!) I received feedback from Michael Niedermayer (you!) to add a fate test and Marvin Scholz about the right way to do it. Great This led to several versions of the patchset over which I received feedback from Lynne about how to properly remove header packets from the demuxer and Andreas Rheinhardt about how to properly pack the binary data from those headers. This was all great, frankly, and I learned a lot. Eventually, the patch set was merged in several waves by you and included a pretty robust fate test application that makes it possible to follow the subsequent change in the ogg demuxer packets and metadata output on each change. However, I had made a mistake on the patch handling ogg/vorbis extradata packing so that one patch was reverted by Andreas Rheinhardt. I communicated with them over IRC and got feedback from Andreas Rheinhardt. on how to properly re-use the avpriv_split_xiph_headers function and its associated binary payload. Again, learned a lot, worked out a fixed patch. Really happy about that. This one patch is the one missing from this series that is preventing the whole series from being properly included in the upcoming release and blocking the rest of the work that I have already done from being submitted. (side note: the rest of work is really much needed, fixing PTS/DTS discontinuity and making ogg streams remuxing work) But, alas, the patch has been waiting for someone to look at it for months now. I had several "verbal" (IRC) commitments from Andreas Rheinhardt that they would review it but it never happened. I truly do not understand why this is happening. The whole saga has now involved 4 seasoned project members. This patch seems safe, it is thoroughly tested thanks to your recommendation about adding fate tests and it follows the recommended guidelines from Lynne and Andreas Rheinhardt. So, what am I missing here? Do I need to refer to the technical committee like Rémi Denis-Courmont suggested? Thanks, -- Romain
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hi Romain On Sun, Aug 03, 2025 at 05:50:17PM -0500, Romain Beauxis wrote:
Le dim. 3 août 2025 à 16:36, Michael Niedermayer <michael@niedermayer.cc> a écrit : [...]
Do you have any advice on how to look for a reviewer for this path?
social media, like twitter
also if it has a positive vibe then we can retweet
Respectfully, this does not make sense to me.
i dont have time ATM, and it seems the others also are overloaded -> so the goal is to attract more reviewing manpower but surely eventually someone will have time and look at it also this is not the most trivial patchset [...]
So, what am I missing here? Do I need to refer to the technical
you can try, but i think that will cause more work and more delays you seem to not understand that i have a release to work on thats months behind, security fixes, a fork i wanted to merge oe cherry pick, and many other things. You can surely see that your patch is not the only thing iam not working on, theres many things i want to work on and do not have the time. The only solution is more manpower. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship: All citizens are under surveillance, all their steps and actions recorded, for the politicians to enforce control. Democracy: All politicians are under surveillance, all their steps and actions recorded, for the citizens to enforce control.
Kieran Kunhya via ffmpeg-devel (HE12025-08-04):
It's really a huge mystery why developers are leaving.
We may never know the answer.
Let me guess. We tell our long-time developers that the new and exciting code they wrote does not belong in the project. We give write access to people who have been there just a few months and have only been moving code around and fixing warnings, and let them think they do not need to listen to advice. We replace an infrastructure for which long-time developers had established shortcuts and productivity tricks with a monster that offers half the features required to do the same in order to attract more drive-by contributors who will never be able to review patches. But I am sure you were thinking of exactly the opposite. Regards, -- Nicolas George
Hi Romain On Mon, Aug 04, 2025 at 02:11:03AM +0200, Michael Niedermayer wrote:
Hi Romain
On Sun, Aug 03, 2025 at 05:50:17PM -0500, Romain Beauxis wrote:
Le dim. 3 août 2025 à 16:36, Michael Niedermayer <michael@niedermayer.cc> a écrit : [...]
Do you have any advice on how to look for a reviewer for this path?
social media, like twitter
also if it has a positive vibe then we can retweet
Respectfully, this does not make sense to me.
i dont have time ATM, and it seems the others also are overloaded
-> so the goal is to attract more reviewing manpower
but surely eventually someone will have time and look at it also this is not the most trivial patchset
I ve reviewed your patch on https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026 now you ignored review comments made previously The patch also contained a trivial mistake: (maybe review your own code if you have time and reviewers are all busy) (also as said previously (IIRC) if reviewers are all busy, help review other peoples patches) priv->comment_size = 0; av_freep(&priv->setup); priv->comment_size = 0; Also its summer vacation time and everyone is busy thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Some Animals are More Equal Than Others. - George Orwell's book Animal Farm
Le lun. 4 août 2025 à 03:21, Michael Niedermayer <michael@niedermayer.cc> a écrit :
Hi Romain
On Mon, Aug 04, 2025 at 02:11:03AM +0200, Michael Niedermayer wrote:
Hi Romain
On Sun, Aug 03, 2025 at 05:50:17PM -0500, Romain Beauxis wrote:
Le dim. 3 août 2025 à 16:36, Michael Niedermayer <michael@niedermayer.cc> a écrit : [...]
Do you have any advice on how to look for a reviewer for this
path?
social media, like twitter
also if it has a positive vibe then we can retweet
Respectfully, this does not make sense to me.
i dont have time ATM, and it seems the others also are overloaded
-> so the goal is to attract more reviewing manpower
but surely eventually someone will have time and look at it also this is not the most trivial patchset
I ve reviewed your patch on https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026 now
Thank for your review and your time. In my previous message I did not mean to direct the whole concern to you in particular.
you ignored review comments made previously
Truly speaking, I did not understand them and/or lost track. I think that I do now and I'm glad that they'll remain on the PR history for future reference.
The patch also contained a trivial mistake: (maybe review your own code if you have time and reviewers are all busy) (also as said previously (IIRC) if reviewers are all busy, help review other peoples patches) priv->comment_size = 0; av_freep(&priv->setup); priv->comment_size = 0;
Thanks for spotting that out. I have addressed all your comments: * Added back legacy ogg vorbis frame decode headers parsing, split them into vorbis_decode_legacy_frame_headers and vorbis_decode_frame_headers for better proofreading. * Split commits into libavf and libavc * Added proper API bump. It looks like the API bump will be required before the release since libavf is already shipping with bitstream modifications on ogg/opus and ogg/flac.
Also its summer vacation time and everyone is busy
I understand that. Should the release be then pushed back to when other people are back to get more people and eyes on what's needed beforehand? Thanks, -- Romain
Hi Romain, I can try to help out here. I am not familiar with the OGG spec but I can at least help with reviews/facilitate to a degree, and it's free so I can read if needed. It seems you are trying to fix issues in and improve OGG support. Can you give me a brief summary of your goals? (i.e. what should I look at first to assist?)
Le lun. 11 août 2025 à 17:31, Yalda <marth64@proxyid.net> a écrit :
Hi Romain,
Hi!
I can try to help out here. I am not familiar with the OGG spec but I can at least help with reviews/facilitate to a degree, and it's free so I can read if needed.
It seems you are trying to fix issues in and improve OGG support. Can you give me a brief summary of your goals? (i.e. what should I look at first to assist?)
Great, thank you so much for taking the time. Got the current work, I would like to improve ffmpeg support for ogg stream chaining. # Ogg chaining The documentation for this feature is here: https://xiph.org/ogg/doc/oggstream.html Ogg stream chaining is used in particular to send a sequence of tracks. Each time a track ends, the current bitstream is terminated and a new one created. This is also required to pass in-band metadata. In practice, ogg chaining works pretty much like concatenating ogg files. Historically, this spec has proven to be a pretty bad design. There are very few tools handling this properly. Most of them treat the end of the first logical bitstream as an end of file. However, and furthermore unfortunately, ogg is still pretty popular for audio streaming, especially using icecast. In particular it is the only container that currently supports lossless codec (flac) with in-band metadata. # In ffmpeg In ffmpeg, support for chained streams is essentially missing. In the following, I'm talking about the state of the code before I started working on it. 1- Most decoders are able to keep decoding after the first logical bitstreams. However, in most cases, secondary and later metadata are lost. 2- The demuxer outputs ogg header packets as ffmpeg packets from secondary and further streams. These packets should be suppressed by the demuxer and instead passed as extradata. 3- PTS and later DTS of secondary and further streams are discontinuous: they restart from their initial value. 4- It is currently not possible to do a ffmpeg -c copy copy of sequentialize ogg streams. I'm trying to fix those 4 points. The work that I have been doing has been focused on the most popular codecs, namely opus, flac and vorbis. I'm happy to extend to more but, at this point, I'd consider all the other one as deprecated personally. Issue #1 is partially addressed with some patches current in the code base and some patches pending. But, first, I'd like to focus on issue #2 because it is almost complete. Currently, issue #2 has been addressed by a series of patches reviewed and merged by Michael and Lynne. 2431fd0b275: introduce a dump utility to track and check changes to the bitstreams on each patch 6d54af6599 and a9d39d6eb9: change the meaning of ogg_codec->packet return value of 1 to make it possible to direct the demuxer to skip some decoded packets At this point, the stage is ready to start skipping ogg header packets: 2fb6416dd0: skip ogg header packets in ogg/flac streams 9c5ed57f94: skip ogg header packets in ogg/opus streams 574f634e49: skip ogg header packets in ogg/vorbis streams The last one was clearly erroneous and reverted by Andreas Rheinhardt The PR here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026 is the fixed version, following Andreas Rheinhardt feedback. The fixed patch packs all 3 vorbis header into a bytestream structure, passes it as extra data and uses it on the decoder side. Following Michael's review, support for demuxed ogg headers is kept as legacy in case libavcodec is linked with an older version of libavformat. A new decoding routine is added to handle the new extradata-based header packets. Let me know if that is enough information for you to help and thanks again! -- Romain
Le lun. 11 août 2025 à 19:21, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Le lun. 11 août 2025 à 17:31, Yalda <marth64@proxyid.net> a écrit :
Hi Romain,
Hi!
I can try to help out here. I am not familiar with the OGG spec but I can at least help with reviews/facilitate to a degree, and it's free so I can read if needed.
It seems you are trying to fix issues in and improve OGG support. Can you give me a brief summary of your goals? (i.e. what should I look at first to assist?)
Great, thank you so much for taking the time.
Got the current work, I would like to improve ffmpeg support for ogg stream chaining.
# Ogg chaining
The documentation for this feature is here: https://xiph.org/ogg/doc/oggstream.html
Ogg stream chaining is used in particular to send a sequence of tracks. Each time a track ends, the current bitstream is terminated and a new one created. This is also required to pass in-band metadata.
In practice, ogg chaining works pretty much like concatenating ogg files.
Historically, this spec has proven to be a pretty bad design. There are very few tools handling this properly. Most of them treat the end of the first logical bitstream as an end of file.
However, and furthermore unfortunately, ogg is still pretty popular for audio streaming, especially using icecast. In particular it is the only container that currently supports lossless codec (flac) with in-band metadata.
# In ffmpeg
In ffmpeg, support for chained streams is essentially missing. In the following, I'm talking about the state of the code before I started working on it.
1- Most decoders are able to keep decoding after the first logical bitstreams. However, in most cases, secondary and later metadata are lost.
2- The demuxer outputs ogg header packets as ffmpeg packets from secondary and further streams. These packets should be suppressed by the demuxer and instead passed as extradata.
3- PTS and later DTS of secondary and further streams are discontinuous: they restart from their initial value.
I meant the other way :-) DTS and then PTS
4- It is currently not possible to do a ffmpeg -c copy copy of sequentialize ogg streams.
I'm trying to fix those 4 points.
The work that I have been doing has been focused on the most popular codecs, namely opus, flac and vorbis. I'm happy to extend to more but, at this point, I'd consider all the other one as deprecated personally.
Issue #1 is partially addressed with some patches current in the code base and some patches pending.
But, first, I'd like to focus on issue #2 because it is almost complete.
Currently, issue #2 has been addressed by a series of patches reviewed and merged by Michael and Lynne.
2431fd0b275: introduce a dump utility to track and check changes to the bitstreams on each patch 6d54af6599 and a9d39d6eb9: change the meaning of ogg_codec->packet return value of 1 to make it possible to direct the demuxer to skip some decoded packets
At this point, the stage is ready to start skipping ogg header packets: 2fb6416dd0: skip ogg header packets in ogg/flac streams 9c5ed57f94: skip ogg header packets in ogg/opus streams 574f634e49: skip ogg header packets in ogg/vorbis streams
The last one was clearly erroneous and reverted by Andreas Rheinhardt
The PR here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20026 is the fixed version, following Andreas Rheinhardt feedback.
The fixed patch packs all 3 vorbis header into a bytestream structure, passes it as extra data and uses it on the decoder side.
Following Michael's review, support for demuxed ogg headers is kept as legacy in case libavcodec is linked with an older version of libavformat.
A new decoding routine is added to handle the new extradata-based header packets.
Let me know if that is enough information for you to help and thanks again! -- Romain
Romain Beauxis: Thank you, Romain, for the clarity. I have some follow up questions just to solidify my understanding. I think this is a good match since this sounds like a segment joining problem which is ironically what I have been doing in principle with my other contributions. 1) Can core stream parameters (channels, sample rate) change mid-flight? 2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me! 3) Is it possible that doing the stream copy is a front-line goal in actuality? In other words, by solving 1/2/3, we are actually wanting to solve 4? (If this thought makes sense) 4) Is there a sample command to spawn such a source stream, or is setting up Icecast with defaults enough and play segments to simulate the conditions?
Re: Yalda (myself)
2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me!
My apologies, you already linked the Xiph doc. I can read that on my own. Inferring the lines, it seems like our current solution is not "handling" them but rather passing them as is.
Hi! Le mar. 12 août 2025 à 11:33, Yalda <marth64@proxyid.net> a écrit :
Romain Beauxis:
Thank you, Romain, for the clarity.
I have some follow up questions just to solidify my understanding. I think this is a good match since this sounds like a segment joining problem which is ironically what I have been doing in principle with my other contributions.
Nice!
1) Can core stream parameters (channels, sample rate) change mid-flight?
In theory, yes. The two streams do not have to have anything in common. In practice, most of the situations where this happens are because the encoder wants to insert an in-band metadata so it's pretty reasonable to assume that encoding parameters are unlikely to change between streams, at least as a first approach.
2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me!
You got the link I see :-) In ogg, there's usually at least 2 to 3 packets: 1. "hello" packet to detect the logical stream content. All first packets of all multiplexed streams are placed inside an initial page. 2. One metadata packet 3. Optionally: one or more codec specific packets (Similarly to considering theora as deprecated, I would also ignore the multiplexing aspect of the problem, at least in a first approach. Ogg streams with audio/video content are also pretty rare these days.) The codec specific packets can contain data required for the decoder. In practice, it seems that in ffmpeg, with ogg/flac and ogg/opus, the decoders are pretty happy continuing their decoding without having to process any new header packet. For opus, there does not seem to be any codec-specific header: https://wiki.xiph.org/OggOpus For flac, the spec says one or more metadata packets and no codec-specific packet: https://xiph.org/flac/ogg_mapping.html For those two codecs, the current libavcodec decoders are pretty happy without those mid-stream headers. With vorbis, the stream has one metadata packet and one codec specific packet that seems required to continue decoding. Thus, the current libavcodec vorbis decoder has to receive and process mid-stream headers, which is why suppressing those from the demuxer output was a trickier task and why this current patch is a hold-out.
3) Is it possible that doing the stream copy is a front-line goal in actuality? In other words, by solving 1/2/3, we are actually wanting to solve 4? (If this thought makes sense)
The most pressing user-facing features are: supporting in-band metadata and copy streams. In-band metadata is just a few commits behind the current pending one. I was looking at them yesterday, they are really super simple. These changes are blocked by the completion of the proper handling of header packets since metadata are passed through them. Supporting copy streams is more tricky as it will require fixing DTS and handling new ogg headers when generating the output streams. I do have most of this sketched out in my local FFmpeg repo.
4) Is there a sample command to spawn such a source stream, or is setting up Icecast with defaults enough and play segments to simulate the conditions?
You can simply encode two ogg/{vorbis, flac, opus} files and concatenate them! I also contributed some short minimal ones to fate, see for instance: ogg-vorbis/chained-meta.ogg -- Romain
Le mar. 12 août 2025 à 14:49, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Hi!
Le mar. 12 août 2025 à 11:33, Yalda <marth64@proxyid.net> a écrit :
Romain Beauxis:
Thank you, Romain, for the clarity.
I have some follow up questions just to solidify my understanding. I think this is a good match since this sounds like a segment joining problem which is ironically what I have been doing in principle with my other contributions.
Nice!
1) Can core stream parameters (channels, sample rate) change mid-flight?
In theory, yes. The two streams do not have to have anything in common.
In practice, most of the situations where this happens are because the encoder wants to insert an in-band metadata so it's pretty reasonable to assume that encoding parameters are unlikely to change between streams, at least as a first approach.
2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me!
You got the link I see :-)
In ogg, there's usually at least 2 to 3 packets: 1. "hello" packet to detect the logical stream content. All first packets of all multiplexed streams are placed inside an initial page. 2. One metadata packet 3. Optionally: one or more codec specific packets
(Similarly to considering theora as deprecated, I would also ignore the multiplexing aspect of the problem, at least in a first approach. Ogg streams with audio/video content are also pretty rare these days.)
The codec specific packets can contain data required for the decoder.
In practice, it seems that in ffmpeg, with ogg/flac and ogg/opus, the decoders are pretty happy continuing their decoding without having to process any new header packet.
For opus, there does not seem to be any codec-specific header: https://wiki.xiph.org/OggOpus
For flac, the spec says one or more metadata packets and no codec-specific packet: https://xiph.org/flac/ogg_mapping.html
For those two codecs, the current libavcodec decoders are pretty happy without those mid-stream headers.
With vorbis, the stream has one metadata packet and one codec specific packet that seems required to continue decoding.
Thus, the current libavcodec vorbis decoder has to receive and process mid-stream headers, which is why suppressing those from the demuxer output was a trickier task and why this current patch is a hold-out.
3) Is it possible that doing the stream copy is a front-line goal in actuality? In other words, by solving 1/2/3, we are actually wanting to solve 4? (If this thought makes sense)
The most pressing user-facing features are: supporting in-band metadata and copy streams.
In-band metadata is just a few commits behind the current pending one. I was looking at them yesterday, they are really super simple.
These changes are blocked by the completion of the proper handling of header packets since metadata are passed through them.
Supporting copy streams is more tricky as it will require fixing DTS and handling new ogg headers when generating the output streams.
I do have most of this sketched out in my local FFmpeg repo.
4) Is there a sample command to spawn such a source stream, or is setting up Icecast with defaults enough and play segments to simulate the conditions?
Sorry I'm realizing you meant a live stream here. You can use liquidsoap, which should be easily installable via the binary packages here: https://github.com/savonet/liquidsoap/releases/tag/v2.3.3 Or using `opam`: https://www.liquidsoap.info/doc-2.3.3/install.html#install-using-opam (make sure to install the vorbis package and also ffmpeg for decoding!) A simple script could be: ```shell % cat icecast-stream.liq s = playlist("/path/to/directory") output.icecast( fallible=true, host="...", # defaults to localhost port=..., # default to 8000 mount="...", # mandatory password="...", # defaults to hackme %ffmpeg(%codec("libmp3lame")), s ) % liquidsoap ./icecast-stream.liq ``` Alternatively you could pick any of the ogg/{opus, flac, vorbis} stream in the xiph directory: https://dir.xiph.org/codecs/Vorbis Thanks, -- Romain
Le mer. 13 août 2025 à 08:44, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Le mar. 12 août 2025 à 14:49, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Hi!
Le mar. 12 août 2025 à 11:33, Yalda <marth64@proxyid.net> a écrit :
Romain Beauxis:
Thank you, Romain, for the clarity.
I have some follow up questions just to solidify my understanding. I think this is a good match since this sounds like a segment joining problem which is ironically what I have been doing in principle with my other contributions.
Nice!
1) Can core stream parameters (channels, sample rate) change mid-flight?
In theory, yes. The two streams do not have to have anything in common.
In practice, most of the situations where this happens are because the encoder wants to insert an in-band metadata so it's pretty reasonable to assume that encoding parameters are unlikely to change between streams, at least as a first approach.
2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me!
You got the link I see :-)
In ogg, there's usually at least 2 to 3 packets: 1. "hello" packet to detect the logical stream content. All first packets of all multiplexed streams are placed inside an initial page. 2. One metadata packet 3. Optionally: one or more codec specific packets
(Similarly to considering theora as deprecated, I would also ignore the multiplexing aspect of the problem, at least in a first approach. Ogg streams with audio/video content are also pretty rare these days.)
The codec specific packets can contain data required for the decoder.
In practice, it seems that in ffmpeg, with ogg/flac and ogg/opus, the decoders are pretty happy continuing their decoding without having to process any new header packet.
For opus, there does not seem to be any codec-specific header: https://wiki.xiph.org/OggOpus
For flac, the spec says one or more metadata packets and no codec-specific packet: https://xiph.org/flac/ogg_mapping.html
For those two codecs, the current libavcodec decoders are pretty happy without those mid-stream headers.
With vorbis, the stream has one metadata packet and one codec specific packet that seems required to continue decoding.
Thus, the current libavcodec vorbis decoder has to receive and process mid-stream headers, which is why suppressing those from the demuxer output was a trickier task and why this current patch is a hold-out.
3) Is it possible that doing the stream copy is a front-line goal in actuality? In other words, by solving 1/2/3, we are actually wanting to solve 4? (If this thought makes sense)
The most pressing user-facing features are: supporting in-band metadata and copy streams.
In-band metadata is just a few commits behind the current pending one. I was looking at them yesterday, they are really super simple.
These changes are blocked by the completion of the proper handling of header packets since metadata are passed through them.
Supporting copy streams is more tricky as it will require fixing DTS and handling new ogg headers when generating the output streams.
I do have most of this sketched out in my local FFmpeg repo.
4) Is there a sample command to spawn such a source stream, or is setting up Icecast with defaults enough and play segments to simulate the conditions?
Sorry I'm realizing you meant a live stream here.
You can use liquidsoap, which should be easily installable via the binary packages here: https://github.com/savonet/liquidsoap/releases/tag/v2.3.3
Or using `opam`: https://www.liquidsoap.info/doc-2.3.3/install.html#install-using-opam (make sure to install the vorbis package and also ffmpeg for decoding!)
A simple script could be:
Huh, sorry: ```shell % cal icecast-playlist.liq 18h 36m 5s 08:45:37 s = playlist("~/sources/test-stream/audio") output.icecast( fallible=true, mount="test", %vorbis, s ) % liquidsoap ./icecast-playlist.liq ```
Alternatively you could pick any of the ogg/{opus, flac, vorbis} stream in the xiph directory: https://dir.xiph.org/codecs/Vorbis
Thanks, -- Romain
Thanks Romain. I am experimenting with this and trying the patches. On Wed, Aug 13, 2025 at 8:46 AM Romain Beauxis <romain.beauxis@gmail.com> wrote:
Le mer. 13 août 2025 à 08:44, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Le mar. 12 août 2025 à 14:49, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Hi!
Le mar. 12 août 2025 à 11:33, Yalda <marth64@proxyid.net> a écrit :
Romain Beauxis:
Thank you, Romain, for the clarity.
I have some follow up questions just to solidify my understanding. I think this is a good match since this sounds like a segment joining problem which is ironically what I have been doing in principle with my other contributions.
Nice!
1) Can core stream parameters (channels, sample rate) change mid-flight?
In theory, yes. The two streams do not have to have anything in common.
In practice, most of the situations where this happens are because the encoder wants to insert an in-band metadata so it's pretty reasonable to assume that encoding parameters are unlikely to change between streams, at least as a first approach.
2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me!
You got the link I see :-)
In ogg, there's usually at least 2 to 3 packets: 1. "hello" packet to detect the logical stream content. All first packets of all multiplexed streams are placed inside an initial page. 2. One metadata packet 3. Optionally: one or more codec specific packets
(Similarly to considering theora as deprecated, I would also ignore the multiplexing aspect of the problem, at least in a first approach. Ogg streams with audio/video content are also pretty rare these days.)
The codec specific packets can contain data required for the decoder.
In practice, it seems that in ffmpeg, with ogg/flac and ogg/opus, the decoders are pretty happy continuing their decoding without having to process any new header packet.
For opus, there does not seem to be any codec-specific header: https://wiki.xiph.org/OggOpus
For flac, the spec says one or more metadata packets and no codec-specific packet: https://xiph.org/flac/ogg_mapping.html
For those two codecs, the current libavcodec decoders are pretty happy without those mid-stream headers.
With vorbis, the stream has one metadata packet and one codec specific packet that seems required to continue decoding.
Thus, the current libavcodec vorbis decoder has to receive and process mid-stream headers, which is why suppressing those from the demuxer output was a trickier task and why this current patch is a hold-out.
3) Is it possible that doing the stream copy is a front-line goal in actuality? In other words, by solving 1/2/3, we are actually wanting to solve 4? (If this thought makes sense)
The most pressing user-facing features are: supporting in-band metadata and copy streams.
In-band metadata is just a few commits behind the current pending one. I was looking at them yesterday, they are really super simple.
These changes are blocked by the completion of the proper handling of header packets since metadata are passed through them.
Supporting copy streams is more tricky as it will require fixing DTS and handling new ogg headers when generating the output streams.
I do have most of this sketched out in my local FFmpeg repo.
4) Is there a sample command to spawn such a source stream, or is setting up Icecast with defaults enough and play segments to simulate the conditions?
Sorry I'm realizing you meant a live stream here.
You can use liquidsoap, which should be easily installable via the binary packages here: https://github.com/savonet/liquidsoap/releases/tag/v2.3.3
Or using `opam`: https://www.liquidsoap.info/doc-2.3.3/install.html#install-using-opam (make sure to install the vorbis package and also ffmpeg for decoding!)
A simple script could be:
Huh, sorry: ```shell % cal icecast-playlist.liq
18h 36m 5s 08:45:37 s = playlist("~/sources/test-stream/audio")
output.icecast( fallible=true, mount="test", %vorbis, s )
% liquidsoap ./icecast-playlist.liq ```
Alternatively you could pick any of the ogg/{opus, flac, vorbis} stream in the xiph directory: https://dir.xiph.org/codecs/Vorbis
Thanks, -- Romain
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Le mer. 20 août 2025 à 15:25, Yalda via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> a écrit :
Thanks Romain. I am experimenting with this and trying the patches.
Awesome. Thanks so much for your time! I have rebased the vorbis header PR against the latest `master` (would love to call it main). I have also pushed the next patches to enable proper metadata parsing so you can see how it would work: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20327 Let me know what you think! -- Romain
On Wed, Aug 13, 2025 at 8:46 AM Romain Beauxis <romain.beauxis@gmail.com> wrote:
Le mer. 13 août 2025 à 08:44, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Le mar. 12 août 2025 à 14:49, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Hi!
Le mar. 12 août 2025 à 11:33, Yalda <marth64@proxyid.net> a écrit :
Romain Beauxis:
Thank you, Romain, for the clarity.
I have some follow up questions just to solidify my understanding. I think this is a good match since this sounds like a segment joining problem which is ironically what I have been doing in principle with my other contributions.
Nice!
1) Can core stream parameters (channels, sample rate) change mid-flight?
In theory, yes. The two streams do not have to have anything in common.
In practice, most of the situations where this happens are because the encoder wants to insert an in-band metadata so it's pretty reasonable to assume that encoding parameters are unlikely to change between streams, at least as a first approach.
2) Why are the header packets emitted to begin with? Are they necessary for the audible bitstream or preamble metadata? Alternatively, a link to external reading is fine by me!
You got the link I see :-)
In ogg, there's usually at least 2 to 3 packets: 1. "hello" packet to detect the logical stream content. All first packets of all multiplexed streams are placed inside an initial page. 2. One metadata packet 3. Optionally: one or more codec specific packets
(Similarly to considering theora as deprecated, I would also ignore the multiplexing aspect of the problem, at least in a first approach. Ogg streams with audio/video content are also pretty rare these days.)
The codec specific packets can contain data required for the decoder.
In practice, it seems that in ffmpeg, with ogg/flac and ogg/opus, the decoders are pretty happy continuing their decoding without having to process any new header packet.
For opus, there does not seem to be any codec-specific header: https://wiki.xiph.org/OggOpus
For flac, the spec says one or more metadata packets and no codec-specific packet: https://xiph.org/flac/ogg_mapping.html
For those two codecs, the current libavcodec decoders are pretty happy without those mid-stream headers.
With vorbis, the stream has one metadata packet and one codec specific packet that seems required to continue decoding.
Thus, the current libavcodec vorbis decoder has to receive and process mid-stream headers, which is why suppressing those from the demuxer output was a trickier task and why this current patch is a hold-out.
3) Is it possible that doing the stream copy is a front-line goal in actuality? In other words, by solving 1/2/3, we are actually wanting to solve 4? (If this thought makes sense)
The most pressing user-facing features are: supporting in-band metadata and copy streams.
In-band metadata is just a few commits behind the current pending one. I was looking at them yesterday, they are really super simple.
These changes are blocked by the completion of the proper handling of header packets since metadata are passed through them.
Supporting copy streams is more tricky as it will require fixing DTS and handling new ogg headers when generating the output streams.
I do have most of this sketched out in my local FFmpeg repo.
4) Is there a sample command to spawn such a source stream, or is setting up Icecast with defaults enough and play segments to simulate the conditions?
Sorry I'm realizing you meant a live stream here.
You can use liquidsoap, which should be easily installable via the binary packages here: https://github.com/savonet/liquidsoap/releases/tag/v2.3.3
Or using `opam`: https://www.liquidsoap.info/doc-2.3.3/install.html#install-using-opam (make sure to install the vorbis package and also ffmpeg for decoding!)
A simple script could be:
Huh, sorry: ```shell % cal icecast-playlist.liq
18h 36m 5s 08:45:37 s = playlist("~/sources/test-stream/audio")
output.icecast( fallible=true, mount="test", %vorbis, s )
% liquidsoap ./icecast-playlist.liq ```
Alternatively you could pick any of the ogg/{opus, flac, vorbis} stream in the xiph directory: https://dir.xiph.org/codecs/Vorbis
Thanks, -- Romain
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hello, Apologies for the delay. I have been testing #20327 today with various samples (local, online) and the api-dump-stream-meta-test tool to observe metadata changes while listening. Code looks good, and to me it seems Romain addressed concerns. I think it makes sense to drop the header packets from output on segment change and store them as extradata. I do believe there might be a leak in the api-dump-stream-meta-test tool (but not in the demuxer change, which is this patch). Compare (valgrind reports errors): ``` valgrind --leak-check=full ./api-dump-stream-meta-test http://play.global.audio/city.ogg ``` vs. ffmpeg itself (clean) ``` valgrind --leak-check=full ./ffmpeg -i http://play.global.audio/city.ogg -c copy -f null - ``` I left a question in the PR, but no other concerns. Note I had found the above input stream from https://dir.xiph.org/codecs/Vorbis Thank you! Yalda
Hi all, Thank you so much for taking the time to look into this, Yalda. I'm glad that you found the changes suitable! I have looked into the memory leak with the api-dump-stream-meta-test. The test binary was not written with a continuous http stream in mind so it needs to be updated to shutdown more gracefully in cases like this. I'll work on it. What do you/y'all think should be the next steps here? If you approve https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20327, can it be merged? I have pushed a rebase of https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20327 which should make it suitable for merge now. Let me know! -- Romain Le sam. 27 sept. 2025 à 15:39, Yalda <marth64@proxyid.net> a écrit :
Hello,
Apologies for the delay. I have been testing #20327 today with various samples (local, online) and the api-dump-stream-meta-test tool to observe metadata changes while listening.
Code looks good, and to me it seems Romain addressed concerns. I think it makes sense to drop the header packets from output on segment change and store them as extradata.
I do believe there might be a leak in the api-dump-stream-meta-test tool (but not in the demuxer change, which is this patch).
Compare (valgrind reports errors): ``` valgrind --leak-check=full ./api-dump-stream-meta-test http://play.global.audio/city.ogg ``` vs. ffmpeg itself (clean) ``` valgrind --leak-check=full ./ffmpeg -i http://play.global.audio/city.ogg -c copy -f null - ```
I left a question in the PR, but no other concerns. Note I had found the above input stream from https://dir.xiph.org/codecs/Vorbis
Thank you! Yalda
Hi Romain, My pleasure and thanks for contributing this change. I was just wanting to know on this line (tagged in PR comments), if there are scenarios where we may only get one of these blocks on segment change and if we need to also emit it to extradata. ``` if (priv->header && priv->comment && priv->setup) { ``` The answer may very well be no and that's fine. Besides that I am willing to approve then merge after a short couple days objections period. The maintainer listed for this file is not active, so I'm not sure there is anyone in particular we need to wait for.
Le lun. 29 sept. 2025 à 11:46, Yalda <marth64@proxyid.net> a écrit :
Hi Romain,
My pleasure and thanks for contributing this change.
I was just wanting to know on this line (tagged in PR comments), if there are scenarios where we may only get one of these blocks on segment change and if we
need to
also emit it to extradata. ``` if (priv->header && priv->comment && priv->setup) { ``` The answer may very well be no and that's fine.
I imagine that there could be situations where a partial header could be useful but this would be required for broken files only. I set this up this way to be more cautious for a first approach. I imagine that, once this matures there might be some documented cases of broken file/stream that would help relaxing this with a clear target/reproduction case.
Besides that I am willing to approve then merge after a short couple days objections period.
This sounds great.
The maintainer listed for this file is not active, so I'm not sure there is anyone in particular we need to wait for.
I would be interested to step in for that if that is something advisable. I have worked with the ogg container for a long time. Thanks again, -- Romain
Romain, Thank you for the clarity. I approved. Will push in ~48 hours if there is no more concerns here.
20327 is merged, thanks. On Mon, Sep 29, 2025 at 1:24 PM Yalda <marth64@proxyid.net> wrote:
Romain,
Thank you for the clarity. I approved. Will push in ~48 hours if there is no more concerns here.
participants (5)
-
Kieran Kunhya -
Michael Niedermayer -
Nicolas George -
Romain Beauxis -
Yalda