[PATCH v4 0/5] Properly decode ogg metadata in ogg/flac and ogg/opus chained bitstreams
This is a series of patches to allow proper decoding of ogg metadata in chained ogg/flac and ogg/opus streams. Changes since v3: * Added opus implementation * Cleaned up, separated patches in a more logical way. Summary of code changes: The changes in this patch series allow proper decoding of metadata associated with subsequent streams in chained ogg/flac and ogg/opus bitstream. This is done by intercepting ogg packets with comments in the ogg demuxer, parsing the comment block and attaching it as packed AV_PKT_DATA_METADATA_UPDATE side-data. The new metadata can then be unpacked in the corresponding decoder and properly added to the first decoded audio frame after the comment packet. It is worth noting that is using a mechanism specific to ogg stream that seemed to only have been used for vorbis streams so far. Along with the changes are new FATE tests validating the implementation. Discussion and context: ogg/opus is a pretty popular combination of codec and encapsulation. In particular, it is widely used in Icecast streams, where chained streams are the norm because of the ogg specs requirement for inserting in-band metadata. ogg/flac streams are pretty important because there are perhaps the only combination of lossless audio codec and open-source container that allows for proper transmittion of lossless audio data accross systems such as Icecast, browser media tags and more. In the context of long-running audio streams, the ogg bitstream specs[1] have historically been very badly implemented. For each new track and each new metadata, the specs require the logical bitstream to come to a full EOF and then start with a full new logical stream. These specs have often been confused with a gobal EOF by most implementations. Furtunately, FFmpeg is a little better at that in that it is able to parse chained logical ogg bitstreams and properly output either encoded ogg packets or decoded audio. Current limitations with chained ogg streams in FFmpeg include: 1. Metadata from secondary chained ogg/flac and ogg/opus bitstreams are ignored by the demuxer/decoder. 2. Secondary chained streams packet headers are silently removed by the demuxer when decoding ogg/opus streams. 3. No adjustment underlying PTS or DTS: PTS and DTS of secondary chained streams reset from their own logical stream initial value causing timestamp discontinuity. 4. Chained bitstreams with more than one underlying type of content (audio+video, etc) is not yet supported though this is a much less needed feature. The changes in this patch series address issues #1 and #2. Future work could address the remaining issues. Thanks, -- Romain [1]: https://xiph.org/ogg/doc/framing.html Romain Beauxis (6): libavformat/oggparseopus.c: Parse extradata from secondary chained streams header packet. tests: Add stream dump test API util. libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata. tests: Add chained ogg/opus stream dump test. libavformat/oggparseflac.c: Parse ogg/flac comments in new ogg packets, add them to ogg stream new_metadata. tests: Add chained ogg/flac stream dump test. libavcodec/flacdec.c | 20 ++- libavcodec/opus/dec.c | 24 ++++ libavformat/oggdec.c | 4 - libavformat/oggparseflac.c | 28 +++++ libavformat/oggparseopus.c | 25 ++++ tests/Makefile | 3 + tests/api/Makefile | 2 +- tests/api/api-dump-stream-meta-test.c | 169 ++++++++++++++++++++++++++ tests/fate/ogg-flac.mak | 11 ++ tests/fate/ogg-opus.mak | 11 ++ 10 files changed, 291 insertions(+), 6 deletions(-) create mode 100644 tests/api/api-dump-stream-meta-test.c create mode 100644 tests/fate/ogg-flac.mak create mode 100644 tests/fate/ogg-opus.mak -- 2.39.5 (Apple Git-154)
These changes make it possible to access the initial header packets of secondary chained ogg/opus bitstreams. libavformat/oggparseopus.c: Parse extradata from secondary chained streams header packet. libavformat/oggdec.c: Do not force ogg stream header parsing on secondary ogg/opus chained streams. libavcodec/opus/dec.c: Ignore opus header packets from secondary chained streams. --- libavcodec/opus/dec.c | 5 +++++ libavformat/oggdec.c | 4 ---- libavformat/oggparseopus.c | 11 +++++++++++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 6c59dc1f46..88a650c81c 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -486,6 +486,11 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int delayed_samples = 0; int i, ret; + if (buf_size > 8 && ( + !memcmp(buf, "OpusHead", 8) || + !memcmp(buf, "OpusTags", 8))) + return buf_size; + /* calculate the number of delayed samples */ for (int i = 0; i < c->p.nb_streams; i++) { OpusStreamContext *s = &c->streams[i]; diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c index 5339fdd32c..4425279ce8 100644 --- a/libavformat/oggdec.c +++ b/libavformat/oggdec.c @@ -239,10 +239,6 @@ static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, char *magic, os->start_trimming = 0; os->end_trimming = 0; - /* Chained files have extradata as a new packet */ - if (codec == &ff_opus_codec) - os->header = -1; - return i; } diff --git a/libavformat/oggparseopus.c b/libavformat/oggparseopus.c index 218e9df581..950b93bd31 100644 --- a/libavformat/oggparseopus.c +++ b/libavformat/oggparseopus.c @@ -125,6 +125,17 @@ static int opus_packet(AVFormatContext *avf, int idx) return AVERROR_INVALIDDATA; } + if (os->psize > 8 && !memcmp(packet, "OpusHead", 8)) { + if ((ret = ff_alloc_extradata(st->codecpar, os->psize)) < 0) + return ret; + + memcpy(st->codecpar->extradata, packet, os->psize); + return 0; + } + + if (os->psize > 8 && !memcmp(packet, "OpusTags", 8)) + return 0; + if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS)) { int seg, d; int duration; -- 2.39.5 (Apple Git-154)
This is the code for the main utility used to dump chained streams and test for decoded metadata in subsequent patches. --- tests/Makefile | 1 + tests/api/Makefile | 2 +- tests/api/api-dump-stream-meta-test.c | 169 ++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 tests/api/api-dump-stream-meta-test.c diff --git a/tests/Makefile b/tests/Makefile index f9f5fc07f3..1f7e5003c2 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -277,6 +277,7 @@ $(FATE_FFPROBE) $(FATE_FFMPEG_FFPROBE) $(FATE_SAMPLES_FFPROBE) $(FATE_SAMPLES_FF $(FATE_SAMPLES_FASTSTART): tools/qt-faststart$(EXESUF) $(FATE_SAMPLES_DUMP_DATA) $(FATE_SAMPLES_DUMP_DATA-yes): tools/venc_data_dump$(EXESUF) $(FATE_SAMPLES_SCALE_SLICE): tools/scale_slice_test$(EXESUF) +$(FATE_SAMPLES_DUMP_STREAM_META): tests/api/api-dump-stream-meta-test$(EXESUF) ifdef SAMPLES FATE += $(FATE_EXTERN) diff --git a/tests/api/Makefile b/tests/api/Makefile index c96e636756..a2cb06a729 100644 --- a/tests/api/Makefile +++ b/tests/api/Makefile @@ -1,7 +1,7 @@ APITESTPROGS-$(call ENCDEC, FLAC, FLAC) += api-flac APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264 APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264-slice -APITESTPROGS-yes += api-seek +APITESTPROGS-yes += api-seek api-dump-stream-meta APITESTPROGS-$(call DEMDEC, H263, H263) += api-band APITESTPROGS-$(HAVE_THREADS) += api-threadmessage APITESTPROGS += $(APITESTPROGS-yes) diff --git a/tests/api/api-dump-stream-meta-test.c b/tests/api/api-dump-stream-meta-test.c new file mode 100644 index 0000000000..4ffdfe8213 --- /dev/null +++ b/tests/api/api-dump-stream-meta-test.c @@ -0,0 +1,169 @@ +/* + * Copyright (c) 2025 Romain Beauxis + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +/** + * Dump stream metadata + */ + +#include "libavcodec/avcodec.h" +#include "libavformat/avformat.h" +#include "libavutil/timestamp.h" + +static int dump_stream_meta(const char *input_filename) +{ + const AVCodec *codec = NULL; + AVPacket *pkt = NULL; + AVFrame *fr = NULL; + AVFormatContext *fmt_ctx = NULL; + AVCodecContext *ctx = NULL; + AVCodecParameters *origin_par = NULL; + int audio_stream; + int result; + char *metadata; + + result = avformat_open_input(&fmt_ctx, input_filename, NULL, NULL); + if (result < 0) { + av_log(NULL, AV_LOG_ERROR, "Can't open file\n"); + return result; + } + + result = avformat_find_stream_info(fmt_ctx, NULL); + if (result < 0) { + av_log(NULL, AV_LOG_ERROR, "Can't get stream info\n"); + goto end; + } + + // TODO: add ability to work with video format + audio_stream = + av_find_best_stream(fmt_ctx, AVMEDIA_TYPE_AUDIO, -1, -1, NULL, 0); + if (audio_stream < 0) { + av_log(NULL, AV_LOG_ERROR, "Can't find audio stream in input file\n"); + result = audio_stream; + goto end; + } + + origin_par = fmt_ctx->streams[audio_stream]->codecpar; + + result = av_dict_get_string( + fmt_ctx->streams[audio_stream]->metadata, &metadata, '=', ':'); + if (result < 0) + goto end; + + printf("Stream ID: %d, codec name: %s, metadata: %s\n", audio_stream, + avcodec_get_name(origin_par->codec_id), metadata); + + codec = avcodec_find_decoder(origin_par->codec_id); + if (!codec) { + av_log(NULL, AV_LOG_ERROR, "Can't find decoder\n"); + result = AVERROR_DECODER_NOT_FOUND; + goto end; + } + + ctx = avcodec_alloc_context3(codec); + if (!ctx) { + av_log(NULL, AV_LOG_ERROR, "Can't allocate decoder context\n"); + result = AVERROR(ENOMEM); + goto end; + } + + result = avcodec_parameters_to_context(ctx, origin_par); + if (result) { + av_log(NULL, AV_LOG_ERROR, "Can't copy decoder context\n"); + goto end; + } + + result = avcodec_open2(ctx, codec, NULL); + if (result < 0) { + av_log(ctx, AV_LOG_ERROR, "Can't open decoder\n"); + goto end; + } + + pkt = av_packet_alloc(); + if (!pkt) { + av_log(NULL, AV_LOG_ERROR, "Cannot allocate packet\n"); + result = AVERROR(ENOMEM); + goto end; + } + + fr = av_frame_alloc(); + if (!fr) { + av_log(NULL, AV_LOG_ERROR, "Can't allocate frame\n"); + result = AVERROR(ENOMEM); + goto end; + } + + for (;;) { + result = av_read_frame(fmt_ctx, pkt); + if (result) + goto end; + + if (pkt->stream_index != audio_stream) { + av_packet_unref(pkt); + continue; + } + + printf("Stream ID: %d, packet PTS: %s, packet DTS: %s\n", + pkt->stream_index, av_ts2str(pkt->pts), av_ts2str(pkt->dts)); + + result = avcodec_send_packet(ctx, pkt); + av_packet_unref(pkt); + + if (result < 0) + goto end; + + result = avcodec_receive_frame(ctx, fr); + if (result == AVERROR_EOF) { + result = 0; + goto end; + } + + if (result == AVERROR(EAGAIN)) + continue; + + result = av_dict_get_string(fr->metadata, &metadata, '=', ':'); + if (result < 0) + goto end; + + printf("Stream ID: %d, frame PTS: %s, metadata: %s\n", + pkt->stream_index, av_ts2str(fr->pts), metadata); + } + +end: + av_packet_free(&pkt); + av_frame_free(&fr); + avformat_close_input(&fmt_ctx); + avcodec_free_context(&ctx); + return result; +} + +int main(int argc, char **argv) +{ + if (argc < 2) { + av_log(NULL, AV_LOG_ERROR, "Incorrect input\n"); + return 1; + } + + if (dump_stream_meta(argv[1]) != AVERROR_EOF) + return 1; + + return 0; +} -- 2.39.5 (Apple Git-154)
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet. They can then be decoded in the opus decoder and attached to the next decoded frame. libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata. libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain; + AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext; @@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata; - if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size; + if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + } + /* calculate the number of delayed samples */ for (int i = 0; i < c->p.nb_streams; i++) { OpusStreamContext *s = &c->streams[i]; @@ -631,6 +645,11 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, frame->nb_samples = decoded_samples; *got_frame_ptr = !!decoded_samples; + if (c->pending_metadata) { + av_dict_copy(&frame->metadata, c->pending_metadata, AV_DICT_APPEND); + av_dict_free(&c->pending_metadata); + } + return avpkt->size; } diff --git a/libavformat/oggparseopus.c b/libavformat/oggparseopus.c index 950b93bd31..3e94a7df3c 100644 --- a/libavformat/oggparseopus.c +++ b/libavformat/oggparseopus.c @@ -116,6 +116,7 @@ static int opus_packet(AVFormatContext *avf, int idx) AVStream *st = avf->streams[idx]; struct oggopus_private *priv = os->private; uint8_t *packet = os->buf + os->pstart; + AVDictionary *new_metadata = NULL; int ret; if (!os->psize) @@ -133,8 +134,21 @@ static int opus_packet(AVFormatContext *avf, int idx) return 0; } - if (os->psize > 8 && !memcmp(packet, "OpusTags", 8)) + if (os->psize > 8 && !memcmp(packet, "OpusTags", 8)) { + ret = ff_vorbis_comment(avf, &new_metadata, os->buf + os->pstart + 8, + os->psize - 8, 1); + + if (ret < 0) + return ret; + + os->new_metadata = av_packet_pack_dictionary(new_metadata, &os->new_metadata_size); + av_dict_free(&new_metadata); + + if (!os->new_metadata) + return AVERROR(ENOMEM); + return 0; + } if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS)) { int seg, d; -- 2.39.5 (Apple Git-154)
On 10/02/2025 20:25, Romain Beauxis wrote:
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet.
They can then be decoded in the opus decoder and attached to the next decoded frame.
libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain;
+ AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext;
@@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata;
- if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size;
+ if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users. What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets. Having to add custom handling to each codec to handle metadata updates is not good design.
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit :
On 10/02/2025 20:25, Romain Beauxis wrote:
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet.
They can then be decoded in the opus decoder and attached to the next decoded frame.
libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain;
+ AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext;
@@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata;
- if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size;
+ if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users.
Thanks for looking into this!
What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing: if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret; os->new_metadata = NULL; os->new_metadata_size = 0; } (Note that this code is already in the ogg decoder and not part of those changes) This is also in response (and personal agreement) with Marvin's feedback here: https://ffmpeg.org/pipermail/ffmpeg-devel/2025-January/338993.html
Having to add custom handling to each codec to handle metadata updates is not good design.
Could you explain more in detail what you mean? I really am confused, the mechanism here is generic, using a flat dictionary stored as AV_PKT_DATA_METADATA_UPDATE extra data and copied over to the frame after decoding. This seems like what you describe? What am I missing? Thanks, -- Romain
On 14/02/2025 00:27, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit :
On 10/02/2025 20:25, Romain Beauxis wrote:
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet.
They can then be decoded in the opus decoder and attached to the next decoded frame.
libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain;
+ AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext;
@@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata;
- if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size;
+ if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users.
Thanks for looking into this!
What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing:
if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret;
os->new_metadata = NULL; os->new_metadata_size = 0; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
(Note that this code is already in the ogg decoder and not part of those changes)
This is also in response (and personal agreement) with Marvin's feedback here: https://ffmpeg.org/pipermail/ffmpeg-devel/2025-January/338993.html
Having to add custom handling to each codec to handle metadata updates is not good design.
Could you explain more in detail what you mean?
I really am confused, the mechanism here is generic, using a flat dictionary stored as AV_PKT_DATA_METADATA_UPDATE extra data and copied over to the frame after decoding.
This seems like what you describe? What am I missing?
You have code in each decoder to handle this. You shouldn't - the generic decode.c framework should forward the updates. Decoders shouldn't care about what a demuxer did, they simply decode.
Le jeu. 13 févr. 2025 à 17:37, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 00:27, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit :
On 10/02/2025 20:25, Romain Beauxis wrote:
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet.
They can then be decoded in the opus decoder and attached to the next decoded frame.
libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain;
+ AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext;
@@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata;
- if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size;
+ if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users.
Thanks for looking into this!
What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing:
if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret;
os->new_metadata = NULL; os->new_metadata_size = 0; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
These strings are part of the expected ogg packeting. The ogg spec mandates that the first packet is a header packet then the following packets are comment packets. OpusHead and OpusTags are the headers indicating such packets. They are part of the original format encapsulation, originally designed for ogg streams. Part of the changes that I intend to do is to surface those packets in the demuxer. In ogg/flac, typically, they are exported by the demuxer and visible when operating at the packet level. With opus, because the header is parsed again, they are swallowed by the demuxer and never surfaced. It is my understanding that, eventually, we want to be able to: - open a demuxer on any ogg stream - open a muxer for a new ogg stream - pass all packets from demuxer to muxer and obtain a valid ogg stream output. There's still some work to do to achieve that with chained ogg streams because of the current PTS/DTS reset but surfacing those packets should be the first step.
(Note that this code is already in the ogg decoder and not part of those changes)
This is also in response (and personal agreement) with Marvin's feedback here: https://ffmpeg.org/pipermail/ffmpeg-devel/2025-January/338993.html
Having to add custom handling to each codec to handle metadata updates is not good design.
Could you explain more in detail what you mean?
I really am confused, the mechanism here is generic, using a flat dictionary stored as AV_PKT_DATA_METADATA_UPDATE extra data and copied over to the frame after decoding.
This seems like what you describe? What am I missing?
You have code in each decoder to handle this. You shouldn't - the generic decode.c framework should forward the updates. Decoders shouldn't care about what a demuxer did, they simply decode.
Thanks, that makes a lot of sense. I have reworked the code to add the update mechanism in libavcodec/decode.c You can see the updated patch set here: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/6#issuecomment-87 Before I submit a new version here, is that a method you agree on? -- Romain
On 14/02/2025 16:48, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 17:37, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 00:27, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit :
On 10/02/2025 20:25, Romain Beauxis wrote:
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet.
They can then be decoded in the opus decoder and attached to the next decoded frame.
libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain;
+ AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext;
@@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata;
- if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size;
+ if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users.
Thanks for looking into this!
What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing:
if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret;
os->new_metadata = NULL; os->new_metadata_size = 0; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
These strings are part of the expected ogg packeting.
The ogg spec mandates that the first packet is a header packet then the following packets are comment packets.
OpusHead and OpusTags are the headers indicating such packets. They are part of the original format encapsulation, originally designed for ogg streams.
Ogg tags and data has exactly no business being in an Opus bitstream. Packets are supposed to contain pure codec data with no container features or data in them.
Part of the changes that I intend to do is to surface those packets in the demuxer.
In ogg/flac, typically, they are exported by the demuxer and visible when operating at the packet level.
With opus, because the header is parsed again, they are swallowed by the demuxer and never surfaced.
It is my understanding that, eventually, we want to be able to: - open a demuxer on any ogg stream - open a muxer for a new ogg stream - pass all packets from demuxer to muxer
and obtain a valid ogg stream output.
There's still some work to do to achieve that with chained ogg streams because of the current PTS/DTS reset but surfacing those packets should be the first step.
We *want* them to be swallowed by the demuxer. You're changing the packet structure by letting them through, breaking users. Even if you weren't, and this was a new codec, you're letting container data through in a codec packet. You do not need to let them through, since you can use the presence of side data to detect this.
Le ven. 14 févr. 2025 à 10:18, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 16:48, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 17:37, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 00:27, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit :
On 10/02/2025 20:25, Romain Beauxis wrote:
These changes parse ogg/opus comment in secondary chained ogg/opus streams and attach them as extradata to the corresponding packet.
They can then be decoded in the opus decoder and attached to the next decoded frame.
libavformat/oggparseopus.c: Parse comments from secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus streams and attach them to the next decoded frame. --- libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- libavformat/oggparseopus.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c index 88a650c81c..cddcefcb5f 100644 --- a/libavcodec/opus/dec.c +++ b/libavcodec/opus/dec.c @@ -125,6 +125,8 @@ typedef struct OpusContext { AVFloatDSPContext *fdsp; float gain;
+ AVDictionary *pending_metadata; + OpusParseContext p; } OpusContext;
@@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, int decoded_samples = INT_MAX; int delayed_samples = 0; int i, ret; + size_t size; + const uint8_t *side_metadata;
- if (buf_size > 8 && ( - !memcmp(buf, "OpusHead", 8) || - !memcmp(buf, "OpusTags", 8))) + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) return buf_size;
+ if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&c->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); + if (ret < 0) + return ret; + } + return buf_size; + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users.
Thanks for looking into this!
What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing:
if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret;
os->new_metadata = NULL; os->new_metadata_size = 0; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
These strings are part of the expected ogg packeting.
The ogg spec mandates that the first packet is a header packet then the following packets are comment packets.
OpusHead and OpusTags are the headers indicating such packets. They are part of the original format encapsulation, originally designed for ogg streams.
Ogg tags and data has exactly no business being in an Opus bitstream. Packets are supposed to contain pure codec data with no container features or data in them.
Part of the changes that I intend to do is to surface those packets in the demuxer.
In ogg/flac, typically, they are exported by the demuxer and visible when operating at the packet level.
With opus, because the header is parsed again, they are swallowed by the demuxer and never surfaced.
It is my understanding that, eventually, we want to be able to: - open a demuxer on any ogg stream - open a muxer for a new ogg stream - pass all packets from demuxer to muxer
and obtain a valid ogg stream output.
There's still some work to do to achieve that with chained ogg streams because of the current PTS/DTS reset but surfacing those packets should be the first step.
We *want* them to be swallowed by the demuxer. You're changing the packet structure by letting them through, breaking users. Even if you weren't, and this was a new codec, you're letting container data through in a codec packet.
You do not need to let them through, since you can use the presence of side data to detect this.
I don't understand what you are suggesting here. Just to make it clear: I am talking here about subsequent ogg header packets in chained ogg streams, not the initial ogg packets. Those packets are required by the spec for chained bitstreams: https://xiph.org/ogg/doc/framing.html How are you suggesting that a user could copy a chained ogg bitstream at the packet level if the header packets of subsequent chained bitstreams are not present? Opus is the only demuxer that will not surface those packets because of a single, codec-specific line of code: https://code.ffmpeg.org/FFmpeg/FFmpeg/src/branch/master/libavformat/oggdec.c... <----> /* Chained files have extradata as a new packet */ if (codec == &ff_opus_codec) os->header = -1; <----> For all other codecs, the header is not parsed again on subsequent chained ogg bitstreams and the packets are passed down to the demuxer. Here's an example output with vorbis. The logs are what's being returned by the demuxer and decoder: Stream ID: 0, codec name: vorbis, metadata: encoder=Lavc61.19.100 libvorbis Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, packet PTS: 128, packet DTS: 128 Stream ID: 0, frame PTS: 128, metadata: encoder=Lavc61.19.100 libvorbis Stream ID: 0, packet PTS: 704, packet DTS: 704 Stream ID: 0, frame PTS: 704, metadata: ---> New packet headers here: Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, packet PTS: 0, packet DTS: 0 ---> New decoded metadata: Stream ID: 0, frame PTS: 0, metadata: encoder=Lavc61.19.100 libvorbis Stream ID: 0, packet PTS: 128, packet DTS: 128 Stream ID: 0, frame PTS: 128, metadata: Stream ID: 0, packet PTS: 704, packet DTS: 704 Stream ID: 0, frame PTS: 704, metadata:
On 14/02/2025 17:50, Romain Beauxis wrote:
Le ven. 14 févr. 2025 à 10:18, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 16:48, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 17:37, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 00:27, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit :
On 10/02/2025 20:25, Romain Beauxis wrote: > These changes parse ogg/opus comment in secondary chained ogg/opus > streams and attach them as extradata to the corresponding packet. > > They can then be decoded in the opus decoder and attached to the next > decoded frame. > > libavformat/oggparseopus.c: Parse comments from > secondary chained ogg/opus streams and pass them as ogg stream new_metadata. > > libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus > streams and attach them to the next decoded frame. > --- > libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- > libavformat/oggparseopus.c | 16 +++++++++++++++- > 2 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c > index 88a650c81c..cddcefcb5f 100644 > --- a/libavcodec/opus/dec.c > +++ b/libavcodec/opus/dec.c > @@ -125,6 +125,8 @@ typedef struct OpusContext { > AVFloatDSPContext *fdsp; > float gain; > > + AVDictionary *pending_metadata; > + > OpusParseContext p; > } OpusContext; > > @@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, > int decoded_samples = INT_MAX; > int delayed_samples = 0; > int i, ret; > + size_t size; > + const uint8_t *side_metadata; > > - if (buf_size > 8 && ( > - !memcmp(buf, "OpusHead", 8) || > - !memcmp(buf, "OpusTags", 8))) > + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) > return buf_size; > > + if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { > + /* New metadata */ > + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); > + if (side_metadata) { > + av_dict_free(&c->pending_metadata); > + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); > + if (ret < 0) > + return ret; > + } > + return buf_size; > + }
This is definitely not the right way to go about it. You're changing the packet layout too, which can potentially break existing users.
Thanks for looking into this!
What you should do instead is to either create a linearized dictionary (e.g. <keylength><key><value_len><value> in a buffer), and pass that as extradata, or add a dictionary field to AVPacket, which would then be copied over to the frame after decoding, similar to how PTS, duration, etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing:
if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret;
os->new_metadata = NULL; os->new_metadata_size = 0; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
These strings are part of the expected ogg packeting.
The ogg spec mandates that the first packet is a header packet then the following packets are comment packets.
OpusHead and OpusTags are the headers indicating such packets. They are part of the original format encapsulation, originally designed for ogg streams.
Ogg tags and data has exactly no business being in an Opus bitstream. Packets are supposed to contain pure codec data with no container features or data in them.
Part of the changes that I intend to do is to surface those packets in the demuxer.
In ogg/flac, typically, they are exported by the demuxer and visible when operating at the packet level.
With opus, because the header is parsed again, they are swallowed by the demuxer and never surfaced.
It is my understanding that, eventually, we want to be able to: - open a demuxer on any ogg stream - open a muxer for a new ogg stream - pass all packets from demuxer to muxer
and obtain a valid ogg stream output.
There's still some work to do to achieve that with chained ogg streams because of the current PTS/DTS reset but surfacing those packets should be the first step.
We *want* them to be swallowed by the demuxer. You're changing the packet structure by letting them through, breaking users. Even if you weren't, and this was a new codec, you're letting container data through in a codec packet.
You do not need to let them through, since you can use the presence of side data to detect this.
Just to make it clear: I am talking here about subsequent ogg header packets in chained ogg streams, not the initial ogg packets.
Those packets are required by the spec for chained bitstreams: https://xiph.org/ogg/doc/framing.html
The muxer/demuxer are responsible for writing and parsing container units. Decoders have nothing to do with them. Encoders only generate extradata and have no concept of containers.
How are you suggesting that a user could copy a chained ogg bitstream at the packet level if the header packets of subsequent chained bitstreams are not present?
Via the extradata update mechanism. AV_PKT_DATA_NEW_EXTRADATA
Opus is the only demuxer that will not surface those packets because of a single, codec-specific line of code: https://code.ffmpeg.org/FFmpeg/FFmpeg/src/branch/master/libavformat/oggdec.c...
<----> /* Chained files have extradata as a new packet */ if (codec == &ff_opus_codec) os->header = -1; <---->
For all other codecs, the header is not parsed again on subsequent chained ogg bitstreams and the packets are passed down to the demuxer.
Here's an example output with vorbis. The logs are what's being returned by the demuxer and decoder:
Then the rest are wrong too. Container details should not have been leaked out into codec packets.
I don't understand what you are suggesting here.
Swallow the extra/metadata. Do not let OpusHead or their equivalents in other codecs inside packets. Emit AV_PKT_DATA_NEW_EXTRADATA from the demuxer instead with the new data, which would contain the OpusHead unit (extradata). In the codec, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, reinit the decoder with it (we should do this in decode.c, but for now, doing it in opusdec.c would be good enough). In the muxer, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, write it to the bitstream. This also removes the need to cache metadata updates. If a packet contains metadata, simply pass that into the AVFrame->metadata once the frame leaves the decoder in decode.c. The packet is not dereferenced yet, so you don't even need to cache it while the packet is being decoded. The Ogg chained "spec" is batshit crazy. I was the one who merged support for it a few years ago. Even the authors admit it was a huge mistake. There isn't a single thing it did properly - from timestamp resets to recommending that stream IDs should be shuffled to requiring that users CRC each potential page to resync. We shouldn't let mistakes of the past break abstraction and turn self-contained codecs into video game codecs which require extensive knowledge of container data.
Le ven. 14 févr. 2025 à 11:55, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 17:50, Romain Beauxis wrote:
Le ven. 14 févr. 2025 à 10:18, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 16:48, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 17:37, Lynne <dev@lynne.ee> a écrit :
On 14/02/2025 00:27, Romain Beauxis wrote:
Le jeu. 13 févr. 2025 à 15:53, Lynne <dev@lynne.ee> a écrit : > > On 10/02/2025 20:25, Romain Beauxis wrote: >> These changes parse ogg/opus comment in secondary chained ogg/opus >> streams and attach them as extradata to the corresponding packet. >> >> They can then be decoded in the opus decoder and attached to the next >> decoded frame. >> >> libavformat/oggparseopus.c: Parse comments from >> secondary chained ogg/opus streams and pass them as ogg stream new_metadata. >> >> libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus >> streams and attach them to the next decoded frame. >> --- >> libavcodec/opus/dec.c | 25 ++++++++++++++++++++++--- >> libavformat/oggparseopus.c | 16 +++++++++++++++- >> 2 files changed, 37 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c >> index 88a650c81c..cddcefcb5f 100644 >> --- a/libavcodec/opus/dec.c >> +++ b/libavcodec/opus/dec.c >> @@ -125,6 +125,8 @@ typedef struct OpusContext { >> AVFloatDSPContext *fdsp; >> float gain; >> >> + AVDictionary *pending_metadata; >> + >> OpusParseContext p; >> } OpusContext; >> >> @@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame, >> int decoded_samples = INT_MAX; >> int delayed_samples = 0; >> int i, ret; >> + size_t size; >> + const uint8_t *side_metadata; >> >> - if (buf_size > 8 && ( >> - !memcmp(buf, "OpusHead", 8) || >> - !memcmp(buf, "OpusTags", 8))) >> + if (buf_size > 8 && !memcmp(buf, "OpusHead", 8)) >> return buf_size; >> >> + if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) { >> + /* New metadata */ >> + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); >> + if (side_metadata) { >> + av_dict_free(&c->pending_metadata); >> + ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata); >> + if (ret < 0) >> + return ret; >> + } >> + return buf_size; >> + } > > This is definitely not the right way to go about it. You're changing the > packet layout too, which can potentially break existing users.
Thanks for looking into this!
> What you should do instead is to either create a linearized dictionary > (e.g. <keylength><key><value_len><value> in a buffer), and pass that as > extradata, or add a dictionary field to AVPacket, which would then be > copied over to the frame after decoding, similar to how PTS, duration, > etc. are carried over from packets.
I'm confused: isn't it exactly what the ogg demuxer is already doing:
if (os->new_metadata) { ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE, os->new_metadata, os->new_metadata_size); if (ret < 0) return ret;
os->new_metadata = NULL; os->new_metadata_size = 0; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
These strings are part of the expected ogg packeting.
The ogg spec mandates that the first packet is a header packet then the following packets are comment packets.
OpusHead and OpusTags are the headers indicating such packets. They are part of the original format encapsulation, originally designed for ogg streams.
Ogg tags and data has exactly no business being in an Opus bitstream. Packets are supposed to contain pure codec data with no container features or data in them.
Part of the changes that I intend to do is to surface those packets in the demuxer.
In ogg/flac, typically, they are exported by the demuxer and visible when operating at the packet level.
With opus, because the header is parsed again, they are swallowed by the demuxer and never surfaced.
It is my understanding that, eventually, we want to be able to: - open a demuxer on any ogg stream - open a muxer for a new ogg stream - pass all packets from demuxer to muxer
and obtain a valid ogg stream output.
There's still some work to do to achieve that with chained ogg streams because of the current PTS/DTS reset but surfacing those packets should be the first step.
We *want* them to be swallowed by the demuxer. You're changing the packet structure by letting them through, breaking users. Even if you weren't, and this was a new codec, you're letting container data through in a codec packet.
You do not need to let them through, since you can use the presence of side data to detect this.
Just to make it clear: I am talking here about subsequent ogg header packets in chained ogg streams, not the initial ogg packets.
Those packets are required by the spec for chained bitstreams: https://xiph.org/ogg/doc/framing.html
The muxer/demuxer are responsible for writing and parsing container units. Decoders have nothing to do with them. Encoders only generate extradata and have no concept of containers.
How are you suggesting that a user could copy a chained ogg bitstream at the packet level if the header packets of subsequent chained bitstreams are not present?
Via the extradata update mechanism. AV_PKT_DATA_NEW_EXTRADATA
Opus is the only demuxer that will not surface those packets because of a single, codec-specific line of code: https://code.ffmpeg.org/FFmpeg/FFmpeg/src/branch/master/libavformat/oggdec.c...
<----> /* Chained files have extradata as a new packet */ if (codec == &ff_opus_codec) os->header = -1; <---->
For all other codecs, the header is not parsed again on subsequent chained ogg bitstreams and the packets are passed down to the demuxer.
Here's an example output with vorbis. The logs are what's being returned by the demuxer and decoder:
Then the rest are wrong too. Container details should not have been leaked out into codec packets.
I don't understand what you are suggesting here.
Swallow the extra/metadata. Do not let OpusHead or their equivalents in other codecs inside packets. Emit AV_PKT_DATA_NEW_EXTRADATA from the demuxer instead with the new data, which would contain the OpusHead unit (extradata). In the codec, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, reinit the decoder with it (we should do this in decode.c, but for now, doing it in opusdec.c would be good enough).
In the muxer, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, write it to the bitstream.
This also removes the need to cache metadata updates. If a packet contains metadata, simply pass that into the AVFrame->metadata once the frame leaves the decoder in decode.c. The packet is not dereferenced yet, so you don't even need to cache it while the packet is being decoded.
This sounds like a plan, thanks I'll get to it.
The Ogg chained "spec" is batshit crazy. I was the one who merged support for it a few years ago. Even the authors admit it was a huge mistake. There isn't a single thing it did properly - from timestamp resets to recommending that stream IDs should be shuffled to requiring that users CRC each potential page to resync. We shouldn't let mistakes of the past break abstraction and turn self-contained codecs into video game codecs which require extensive knowledge of container data.
Oh I am not here to argue in favor of this spec haha. The number of times we had to face broken implementation of chained ogg bitstreams in liquidsoap is.. ho my.. That was a mistake but we can try to at least support it well. Thanks for the pointers, I'll be back with what might be a bigger chunk of changes then. -- Romain
Hi again, Le ven. 14 févr. 2025 à 12:08, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Le ven. 14 févr. 2025 à 11:55, Lynne <dev@lynne.ee> a écrit :
Swallow the extra/metadata. Do not let OpusHead or their equivalents in other codecs inside packets. Emit AV_PKT_DATA_NEW_EXTRADATA from the demuxer instead with the new data, which would contain the OpusHead unit (extradata). In the codec, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, reinit the decoder with it (we should do this in decode.c, but for now, doing it in opusdec.c would be good enough).
In the muxer, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, write it to the bitstream.
This also removes the need to cache metadata updates. If a packet contains metadata, simply pass that into the AVFrame->metadata once the frame leaves the decoder in decode.c. The packet is not dereferenced yet, so you don't even need to cache it while the packet is being decoded.
This sounds like a plan, thanks I'll get to it.
I'm looking into this again. Here are some notes: * Frames can potentially span several ogg packets so metadata should be cached in case a packet does not immediately return a decoded frame * The problem is extra header packets in the demuxed bitstream is orthogonal to the problem is passing new metadata * It seems more clear to use AV_PKT_DATA_METADATA_UPDATE to pass metadata updates and AV_PKT_DATA_NEW_EXTRADATA to pass new header packets Thus, I would like to divide up the work in the following order: 1. First a patcheset that adds a generic method to insert new metadata to decoded frames using AV_PKT_DATA_NEW_EXTRADATA. These changes wouldn't change the demuxing logic currently in place. 2. Then a patchset to start adding ogg packet headers from secondary chained streams as AV_PKT_DATA_NEW_EXTRADATA and suppress them from the bitstream #2 might need some iterations, maybe one series that stash them header packets and hides them from the demuxer and another one that starts dumping them into the bitstream in the muxer. The reason being that, in order to be able to dump them in the muxer, we will also need to revise the PTS/DTS logic and that seems like a lot of work for one single patch series. How does the plan feel for you? -- Romain
Le dim. 16 févr. 2025 à 15:02, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Hi again,
Le ven. 14 févr. 2025 à 12:08, Romain Beauxis <romain.beauxis@gmail.com> a écrit :
Le ven. 14 févr. 2025 à 11:55, Lynne <dev@lynne.ee> a écrit :
Swallow the extra/metadata. Do not let OpusHead or their equivalents
other codecs inside packets. Emit AV_PKT_DATA_NEW_EXTRADATA from the demuxer instead with the new data, which would contain the OpusHead unit (extradata). In the codec, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, reinit
decoder with it (we should do this in decode.c, but for now, doing it in opusdec.c would be good enough).
In the muxer, if a packet contains AV_PKT_DATA_NEW_EXTRADATA, write it to the bitstream.
This also removes the need to cache metadata updates. If a packet contains metadata, simply pass that into the AVFrame->metadata once
in the the
frame leaves the decoder in decode.c. The packet is not dereferenced yet, so you don't even need to cache it while the packet is being decoded.
This sounds like a plan, thanks I'll get to it.
I'm looking into this again. Here are some notes:
* Frames can potentially span several ogg packets so metadata should be cached in case a packet does not immediately return a decoded frame * The problem is extra header packets in the demuxed bitstream is orthogonal to the problem is passing new metadata * It seems more clear to use AV_PKT_DATA_METADATA_UPDATE to pass metadata updates and AV_PKT_DATA_NEW_EXTRADATA to pass new header packets
Thus, I would like to divide up the work in the following order: 1. First a patcheset that adds a generic method to insert new metadata to decoded frames using AV_PKT_DATA_NEW_EXTRADATA. These changes wouldn't change the demuxing logic currently in place.
Sorry I meant AV_PKT_DATA_METADATA_UPDATE in the above.
2. Then a patchset to start adding ogg packet headers from secondary chained streams as AV_PKT_DATA_NEW_EXTRADATA and suppress them from the bitstream
#2 might need some iterations, maybe one series that stash the header packets and hides them from the demuxer and another one that starts dumping them into the bitstream in the muxer. The reason being that, in order to be able to dump them in the muxer, we will also need to revise the PTS/DTS logic and that seems like a lot of work for one single patch series.
How does the plan feel for you? -- Romain
This adds the test to confirm that secondary chained ogg/opus streams are properly decoded. Using the test output, we can confirm that secondary stream header packets are properly passed down and that the new metadata are properly parsed. Output before the changes: Stream ID: 0, codec name: opus, metadata: encoder=Lavc61.19.100 libopus:title=First Stream Stream ID: 0, packet PTS: -312, packet DTS: -312 Stream ID: 0, frame PTS: -312, metadata: Stream ID: 0, packet PTS: 648, packet DTS: 648 Stream ID: 0, frame PTS: 648, metadata: Stream ID: 0, packet PTS: 1608, packet DTS: 1608 Stream ID: 0, frame PTS: 1608, metadata: Stream ID: 0, packet PTS: 2568, packet DTS: 2568 Stream ID: 0, frame PTS: 2568, metadata: Stream ID: 0, packet PTS: 3528, packet DTS: 3528 Stream ID: 0, frame PTS: 3528, metadata: Stream ID: 0, packet PTS: 4488, packet DTS: 4488 Stream ID: 0, frame PTS: 4488, metadata: Stream ID: 0, packet PTS: -312, packet DTS: -312 Stream ID: 0, frame PTS: -312, metadata: Stream ID: 0, packet PTS: 648, packet DTS: 648 Stream ID: 0, frame PTS: 648, metadata: Stream ID: 0, packet PTS: 1608, packet DTS: 1608 Stream ID: 0, frame PTS: 1608, metadata: Stream ID: 0, packet PTS: 2568, packet DTS: 2568 Stream ID: 0, frame PTS: 2568, metadata: Stream ID: 0, packet PTS: 3528, packet DTS: 3528 Stream ID: 0, frame PTS: 3528, metadata: Stream ID: 0, packet PTS: 4488, packet DTS: 4488 Stream ID: 0, frame PTS: 4488, metadata: Output after the changes: Stream ID: 0, codec name: opus, metadata: encoder=Lavc61.19.100 libopus:title=First Stream Stream ID: 0, packet PTS: -312, packet DTS: -312 Stream ID: 0, frame PTS: -312, metadata: Stream ID: 0, packet PTS: 648, packet DTS: 648 Stream ID: 0, frame PTS: 648, metadata: Stream ID: 0, packet PTS: 1608, packet DTS: 1608 Stream ID: 0, frame PTS: 1608, metadata: Stream ID: 0, packet PTS: 2568, packet DTS: 2568 Stream ID: 0, frame PTS: 2568, metadata: Stream ID: 0, packet PTS: 3528, packet DTS: 3528 Stream ID: 0, frame PTS: 3528, metadata: Stream ID: 0, packet PTS: 4488, packet DTS: 4488 Stream ID: 0, frame PTS: 4488, metadata: Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, packet PTS: -312, packet DTS: -312 Stream ID: 0, frame PTS: -312, metadata: encoder=Lavc61.19.100 libopus:title=Second Stream Stream ID: 0, packet PTS: 648, packet DTS: 648 Stream ID: 0, frame PTS: 648, metadata: Stream ID: 0, packet PTS: 1608, packet DTS: 1608 Stream ID: 0, frame PTS: 1608, metadata: Stream ID: 0, packet PTS: 2568, packet DTS: 2568 Stream ID: 0, frame PTS: 2568, metadata: Stream ID: 0, packet PTS: 3528, packet DTS: 3528 Stream ID: 0, frame PTS: 3528, metadata: Stream ID: 0, packet PTS: 4488, packet DTS: 4488 Stream ID: 0, frame PTS: 4488, metadata: --- tests/Makefile | 1 + tests/fate/ogg-opus.mak | 11 +++++++++++ 2 files changed, 12 insertions(+) create mode 100644 tests/fate/ogg-opus.mak diff --git a/tests/Makefile b/tests/Makefile index 1f7e5003c2..5ba12e3f3f 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -219,6 +219,7 @@ include $(SRC_PATH)/tests/fate/mpeg4.mak include $(SRC_PATH)/tests/fate/mpegps.mak include $(SRC_PATH)/tests/fate/mpegts.mak include $(SRC_PATH)/tests/fate/mxf.mak +include $(SRC_PATH)/tests/fate/ogg-opus.mak include $(SRC_PATH)/tests/fate/oma.mak include $(SRC_PATH)/tests/fate/opus.mak include $(SRC_PATH)/tests/fate/pcm.mak diff --git a/tests/fate/ogg-opus.mak b/tests/fate/ogg-opus.mak new file mode 100644 index 0000000000..75cb15bc05 --- /dev/null +++ b/tests/fate/ogg-opus.mak @@ -0,0 +1,11 @@ +FATE_OGG_OPUS += fate-ogg-opus-chained-meta +fate-ogg-opus-chained-meta: REF = $(SAMPLES)/ogg-opus/chained-meta.txt +fate-ogg-opus-chained-meta: CMD = $(APITESTSDIR)/api-dump-stream-meta-test$(EXESUF) $(TARGET_SAMPLES)/ogg-opus/chained-meta.ogg + +FATE_OGG_OPUS-$(call DEMDEC, OGG, OPUS) += $(FATE_OGG_OPUS) + +FATE_SAMPLES_DUMP_STREAM_META += $(FATE_OGG_OPUS-yes) + +FATE_EXTERN += $(FATE_OGG_OPUS-yes) + +fate-ogg-opus: $(FATE_OGG_OPUS-yes) -- 2.39.5 (Apple Git-154)
This the same changes as done with ogg/opus: parse comments in secondary chained ogg/flac streams, attach them as packed extradata, decode and attach them to the next decoded stream in the flac decoder. libavformat/oggparseflac.c: Parse ogg/flac comments in new ogg packets, add them to ogg stream new_metadata. libavcodec/flacdec.c: Process AV_PKT_DATA_METADATA_UPDATE on new packets, add them as new metadata on the new decoded audio frame. --- libavcodec/flacdec.c | 20 +++++++++++++++++++- libavformat/oggparseflac.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c index ad921a1bd1..337f3e1702 100644 --- a/libavcodec/flacdec.c +++ b/libavcodec/flacdec.c @@ -68,6 +68,8 @@ typedef struct FLACContext { unsigned int decoded_buffer_size_33bps; int buggy_lpc; ///< use workaround for old lavc encoded files + AVDictionary *pending_metadata; + FLACDSPContext dsp; } FLACContext; @@ -718,6 +720,8 @@ static int flac_decode_frame(AVCodecContext *avctx, AVFrame *frame, int buf_size = avpkt->size; FLACContext *s = avctx->priv_data; int bytes_read = 0; + const uint8_t *side_metadata; + size_t size; int ret; *got_frame_ptr = 0; @@ -728,7 +732,14 @@ static int flac_decode_frame(AVCodecContext *avctx, AVFrame *frame, } if (buf_size > 0 && (*buf & 0x7F) == FLAC_METADATA_TYPE_VORBIS_COMMENT) { - av_log(s->avctx, AV_LOG_DEBUG, "skipping vorbis comment\n"); + /* New metadata */ + side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size); + if (side_metadata) { + av_dict_free(&s->pending_metadata); + ret = av_packet_unpack_dictionary(side_metadata, size, &s->pending_metadata); + if (ret < 0) + return ret; + } return buf_size; } @@ -788,6 +799,12 @@ static int flac_decode_frame(AVCodecContext *avctx, AVFrame *frame, buf_size - bytes_read, buf_size); } + + if (s->pending_metadata) { + av_dict_copy(&frame->metadata, s->pending_metadata, AV_DICT_APPEND); + av_dict_free(&s->pending_metadata); + } + *got_frame_ptr = 1; return bytes_read; @@ -799,6 +816,7 @@ static av_cold int flac_decode_close(AVCodecContext *avctx) av_freep(&s->decoded_buffer); av_freep(&s->decoded_buffer_33bps); + av_dict_free(&s->pending_metadata); return 0; } diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c index f25ed9cc15..3810806112 100644 --- a/libavformat/oggparseflac.c +++ b/libavformat/oggparseflac.c @@ -78,6 +78,32 @@ flac_header (AVFormatContext * s, int idx) return 1; } +static int +flac_packet (AVFormatContext * s, int idx) +{ + struct ogg *ogg = s->priv_data; + struct ogg_stream *os = ogg->streams + idx; + AVDictionary *new_metadata = NULL; + int ret; + + if (os->psize > 0 && os->buf[os->pstart] && + (os->buf[os->pstart] & 0x7F) == FLAC_METADATA_TYPE_VORBIS_COMMENT) { + ret = ff_vorbis_comment(s, &new_metadata, os->buf + os->pstart + 4, + os->psize - 4, 1); + + if (ret < 0) + return ret; + + os->new_metadata = av_packet_pack_dictionary(new_metadata, &os->new_metadata_size); + av_dict_free(&new_metadata); + + if (!os->new_metadata) + return AVERROR(ENOMEM); + } + + return 0; +} + static int old_flac_header (AVFormatContext * s, int idx) { @@ -130,6 +156,7 @@ const struct ogg_codec ff_flac_codec = { .magic = "\177FLAC", .magicsize = 5, .header = flac_header, + .packet = flac_packet, .nb_header = 2, }; @@ -137,5 +164,6 @@ const struct ogg_codec ff_old_flac_codec = { .magic = "fLaC", .magicsize = 4, .header = old_flac_header, + .packet = flac_packet, .nb_header = 0, }; -- 2.39.5 (Apple Git-154)
Same test as for ogg/opus. Samples and output for both tests are available here: https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0 In this case, the secondary chained ogg/flac header packets are already visible before the changes. Before the changes: Stream ID: 0, codec name: flac, metadata: encoder=Lavc61.19.100 flac:title=First Stream Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, frame PTS: 0, metadata: Stream ID: 0, packet PTS: 4608, packet DTS: 4608 Stream ID: 0, frame PTS: 4608, metadata: Stream ID: 0, packet PTS: 0, packet DTS: 0 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: Stream ID: 0, packet PTS: 4608, packet DTS: 4608 Stream ID: 0, frame PTS: 4608, metadata: After the changes: Stream ID: 0, codec name: flac, metadata: encoder=Lavc61.19.100 flac:title=First Stream Stream ID: 0, packet PTS: 0, packet DTS: 0 Stream ID: 0, frame PTS: 0, metadata: Stream ID: 0, packet PTS: 4608, packet DTS: 4608 Stream ID: 0, frame PTS: 4608, metadata: Stream ID: 0, packet PTS: 0, packet DTS: 0 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: encoder=Lavc61.19.100 flac:title=Second Stream Stream ID: 0, packet PTS: 4608, packet DTS: 4608 Stream ID: 0, frame PTS: 4608, metadata: --- tests/Makefile | 1 + tests/fate/ogg-flac.mak | 11 +++++++++++ 2 files changed, 12 insertions(+) create mode 100644 tests/fate/ogg-flac.mak diff --git a/tests/Makefile b/tests/Makefile index 5ba12e3f3f..7d16f7fe16 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -220,6 +220,7 @@ include $(SRC_PATH)/tests/fate/mpegps.mak include $(SRC_PATH)/tests/fate/mpegts.mak include $(SRC_PATH)/tests/fate/mxf.mak include $(SRC_PATH)/tests/fate/ogg-opus.mak +include $(SRC_PATH)/tests/fate/ogg-flac.mak include $(SRC_PATH)/tests/fate/oma.mak include $(SRC_PATH)/tests/fate/opus.mak include $(SRC_PATH)/tests/fate/pcm.mak diff --git a/tests/fate/ogg-flac.mak b/tests/fate/ogg-flac.mak new file mode 100644 index 0000000000..22e2030534 --- /dev/null +++ b/tests/fate/ogg-flac.mak @@ -0,0 +1,11 @@ +FATE_OGG_FLAC += fate-ogg-flac-chained-meta +fate-ogg-flac-chained-meta: REF = $(SAMPLES)/ogg-flac/chained-meta.txt +fate-ogg-flac-chained-meta: CMD = $(APITESTSDIR)/api-dump-stream-meta-test$(EXESUF) $(TARGET_SAMPLES)/ogg-flac/chained-meta.ogg + +FATE_OGG_FLAC-$(call DEMDEC, OGG, FLAC) += $(FATE_OGG_FLAC) + +FATE_SAMPLES_DUMP_STREAM_META += $(FATE_OGG_FLAC-yes) + +FATE_EXTERN += $(FATE_OGG_FLAC-yes) + +fate-ogg-flac: $(FATE_OGG_FLAC-yes) -- 2.39.5 (Apple Git-154)
participants (2)
-
Lynne -
Romain Beauxis