[FFmpeg-devel] [PATCH v4 0/5] Properly decode ogg metadata in ogg/flac and ogg/opus chained bitstreams

[FFmpeg-devel] [PATCH v4 0/5] Properly decode ogg metadata in ogg/flac and ogg/opus chained bitstreams

[Romain Beauxis]

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)

_______________________________________________
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] [PATCH v4 1/6] Pass ogg/opus secondary header packets to the

[Romain Beauxis]

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)

_______________________________________________
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] [PATCH v4 2/6] tests: Add stream dump test API util.

[Romain Beauxis]

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)

_______________________________________________
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] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Romain Beauxis]

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)

_______________________________________________
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] [PATCH v4 4/6] tests: Add chained ogg/opus stream dump test.

[Romain Beauxis]

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)

_______________________________________________
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] [PATCH v4 5/6] Parse secondary chained ogg/flac stream comments.

[Romain Beauxis]

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)

_______________________________________________
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] [PATCH v4 6/6] tests: Add chained ogg/flac stream dump test.

[Romain Beauxis]

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)

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Lynne]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2615 bytes --]

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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Romain Beauxis]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Lynne]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4136 bytes --]



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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Romain Beauxis]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Lynne]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 5179 bytes --]



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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Romain Beauxis]

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#L242-L244

<---->
    /* 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:
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Lynne]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 8327 bytes --]

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#L242-L244
> 
> <---->
>      /* 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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Romain Beauxis]

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#L242-L244
> >
> > <---->
> >      /* 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
_______________________________________________
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".