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

[FFmpeg-devel] [PATCH v5 0/6] 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 last version:
- Opus processing moved back to `opus_header` to make sure that opus
  demuxing is not impacted by those changes.
- Generic decoding method implemented directly in `libavcodec/decode.c` to avoid
  having each decoded handle the metadata injection.

### 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 `libavcodec/decode.c` and attached to
the next decoded frame.

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 initia 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 issue #1.

Future work could address the remaining issues.
-- Romain

[1]: https://xiph.org/ogg/doc/framing.html

Romain Beauxis (6):
  libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet
    extra data, attach them to the next decoded frame.
  tests: Add stream dump test API util.
  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.
  libavformat/oggparseopus.c: Parse comments from secondary chained
    streams header packet.
  tests: Add chained ogg/opus stream dump test.

 libavcodec/decode.c                   |  20 +++
 libavformat/oggparseflac.c            |  28 +++++
 libavformat/oggparseopus.c            |  21 +++-
 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 ++
 8 files changed, 263 insertions(+), 2 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 v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Romain Beauxis]

libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
attach them to the next decoded frame.

The metadata needs to be cached because there is no guarantee that each packet
will generate a decoded frame immediately.

`AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
so regression risks seem low.

This generic mechanism could be reused to support more in-band metadata update
in the future.

---
 libavcodec/decode.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index cac7e620d2..96e2f0ce95 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -97,6 +97,8 @@ typedef struct DecodeContext {
     int lcevc_frame;
     int width;
     int height;
+
+    AVDictionary *pending_metadata;
 } DecodeContext;
 
 static DecodeContext *decode_ctx(AVCodecInternal *avci)
@@ -729,6 +731,8 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
 {
     AVCodecInternal *avci = avctx->internal;
     DecodeContext     *dc = decode_ctx(avci);
+    const uint8_t *side_metadata;
+    size_t size;
     int ret;
 
     if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
@@ -746,6 +750,14 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
         ret = av_packet_ref(avci->buffer_pkt, avpkt);
         if (ret < 0)
             return ret;
+
+        side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size);
+        if (side_metadata) {
+            av_dict_free(&dc->pending_metadata);
+            ret = av_packet_unpack_dictionary(side_metadata, size, &dc->pending_metadata);
+            if (ret < 0)
+                return ret;
+        }
     } else
         dc->draining_started = 1;
 
@@ -815,6 +827,7 @@ fail:
 int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
 {
     AVCodecInternal *avci = avctx->internal;
+    DecodeContext     *dc = decode_ctx(avci);
     int ret;
 
     if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
@@ -887,6 +900,12 @@ int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
         }
     }
 #endif
+
+    if (dc->pending_metadata) {
+        av_dict_copy(&frame->metadata, dc->pending_metadata, AV_DICT_APPEND);
+        av_dict_free(&dc->pending_metadata);
+    }
+
     return 0;
 fail:
     av_frame_unref(frame);
@@ -2314,4 +2333,5 @@ void ff_decode_internal_uninit(AVCodecContext *avctx)
     DecodeContext *dc = decode_ctx(avci);
 
     av_refstruct_unref(&dc->lcevc);
+    av_dict_free(&dc->pending_metadata);
 }
-- 
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 v5 2/6] tests: Add stream dump test API util.

[Romain Beauxis]

This is the main test utility used in subsequent new FATE tests. It dumps
DTS/PTS of packets and decoded frames along with their metadata.

---
 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 v5 3/6] Parse ogg/flac comments in new ogg packets

[Romain Beauxis]

libavformat/oggparseflac.c: Parse ogg/flac comments in new ogg packets, add them to ogg stream new_metadata.

This is pretty straight forward and taps into a mechanism already in place in
libavformat/oggdec.c

---
 libavformat/oggparseflac.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

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 v5 4/6] tests: Add chained ogg/flac stream dump test.

[Romain Beauxis]

Implements fate test for decoding chainged ogg/flac metadata.

* Demuxing is not modified
* Only change is added frame metadata.

Samples are here: https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0

Before 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::n

After 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 1f7e5003c2..66e5189e6d 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-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".

[FFmpeg-devel] [PATCH v5 5/6] Parse comments from secondary chained ogg/opus streams

[Romain Beauxis]

libavformat/oggparseopus.c: Parse comments from secondary chained streams header
packet.

This is the same processing as with ogg/flac except that is done in the
`opus_header` handler.

This is because, in the case of opus streams, the header is currently parsed
again on secondary chained streams so this is where comments have to be
extracted too.

---
 libavformat/oggparseopus.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/libavformat/oggparseopus.c b/libavformat/oggparseopus.c
index 218e9df581..76c51d2961 100644
--- a/libavformat/oggparseopus.c
+++ b/libavformat/oggparseopus.c
@@ -29,6 +29,7 @@
 
 struct oggopus_private {
     int need_comments;
+    int comments_parsed;
     unsigned pre_skip;
     int64_t cur_dts;
 };
@@ -43,6 +44,7 @@ static int opus_header(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 (!priv) {
@@ -81,7 +83,24 @@ static int opus_header(AVFormatContext *avf, int idx)
     if (priv->need_comments) {
         if (os->psize < 8 || memcmp(packet, "OpusTags", 8))
             return AVERROR_INVALIDDATA;
-        ff_vorbis_stream_comment(avf, st, packet + 8, os->psize - 8);
+
+        if (!priv->comments_parsed) {
+            ff_vorbis_stream_comment(avf, st, packet + 8, os->psize - 8);
+            priv->comments_parsed = 1;
+        } else {
+            ret = ff_vorbis_comment(avf, &new_metadata, packet + 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);
+        }
+
         priv->need_comments--;
         return 1;
     }
-- 
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 v5 6/6] tests: Add chained ogg/opus stream dump test.

[Romain Beauxis]

Implements fate test for decoding chainged ogg/opus metadata.

* Demuxing is not modified
* Only change is added frame metadata.

Samples are here: https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0

Before 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:

After 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: 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 66e5189e6d..128b194291 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-flac.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".

Re: [FFmpeg-devel] [PATCH v5 0/6] Properly decode ogg metadata in ogg/flac and ogg/opus chained bitstreams

[Romain Beauxis]

Le lun. 17 févr. 2025 à 10:19, Romain Beauxis
<romain.beauxis@gmail.com> a écrit :
>
> This is a series of patches to allow proper decoding of ogg metadata in chained
> `ogg/flac` and `ogg/opus` streams.

I forgot to add: this patch set is also available for review at:
https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/6

> ## Changes since last version:
> - Opus processing moved back to `opus_header` to make sure that opus
>   demuxing is not impacted by those changes.
> - Generic decoding method implemented directly in `libavcodec/decode.c` to avoid
>   having each decoded handle the metadata injection.
>
> ### 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 `libavcodec/decode.c` and attached to
> the next decoded frame.
>
> 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 initia 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 issue #1.
>
> Future work could address the remaining issues.
> -- Romain
>
> [1]: https://xiph.org/ogg/doc/framing.html
>
> Romain Beauxis (6):
>   libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet
>     extra data, attach them to the next decoded frame.
>   tests: Add stream dump test API util.
>   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.
>   libavformat/oggparseopus.c: Parse comments from secondary chained
>     streams header packet.
>   tests: Add chained ogg/opus stream dump test.
>
>  libavcodec/decode.c                   |  20 +++
>  libavformat/oggparseflac.c            |  28 +++++
>  libavformat/oggparseopus.c            |  21 +++-
>  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 ++
>  8 files changed, 263 insertions(+), 2 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".

Re: [FFmpeg-devel] [PATCH v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Lynne]

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

On 17/02/2025 17:19, Romain Beauxis wrote:
> libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
> attach them to the next decoded frame.
> 
> The metadata needs to be cached because there is no guarantee that each packet
> will generate a decoded frame immediately.
> 
> `AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
> so regression risks seem low.
> 
> This generic mechanism could be reused to support more in-band metadata update
> in the future.

Much cleaner than before, thanks.
Which packets will not generate a decoded frame immediately? When it 
comes to audio, all of them should output something immediately.
The code is correct, metadata updates could occur on B-frames in video. 
Just wondering.

[-- 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 v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Romain Beauxis]

Le lun. 17 févr. 2025 à 13:10, Lynne <dev@lynne.ee> a écrit :
>
> On 17/02/2025 17:19, Romain Beauxis wrote:
> > libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
> > attach them to the next decoded frame.
> >
> > The metadata needs to be cached because there is no guarantee that each packet
> > will generate a decoded frame immediately.
> >
> > `AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
> > so regression risks seem low.
> >
> > This generic mechanism could be reused to support more in-band metadata update
> > in the future.
>
> Much cleaner than before, thanks.

Thanks for the guidance!

> Which packets will not generate a decoded frame immediately? When it
> comes to audio, all of them should output something immediately.
> The code is correct, metadata updates could occur on B-frames in video.
> Just wondering.

I was working from memory about ogg/flac but got it wrong: flac frames
map to ogg packets BUT they can span page boundaries:
> * Native FLAC audio frames appear as subsequent packets in the stream. Each packet corresponds to one FLAC audio frame. The first byte of each packet serves as the packet type. >.   Since audio packets are native FLAC frames, this first byte will be always 0xFF according to the native FLAC format specification.
> * The last page is marked 'end of stream' in the page flags.
> * FLAC packets may span page boundaries.

Source: https://xiph.org/flac/ogg_mapping.html

This is a bit confusing, though.

At any rate, however, in the current implementation, flac ogg packet
headers are still exported by the demuxer and later ignored by the
decoder.

These packets would not generate an audio frame.

I'm happy to work on further improvement on this as discussed. Then we
could consider un-caching the metadata.

-- 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 v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Lynne]

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

On 17/02/2025 17:19, Romain Beauxis wrote:
> libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
> attach them to the next decoded frame.
> 
> The metadata needs to be cached because there is no guarantee that each packet
> will generate a decoded frame immediately.
> 
> `AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
> so regression risks seem low.
> 
> This generic mechanism could be reused to support more in-band metadata update
> in the future.
> 
> ---
>   libavcodec/decode.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index cac7e620d2..96e2f0ce95 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -97,6 +97,8 @@ typedef struct DecodeContext {
>       int lcevc_frame;
>       int width;
>       int height;
> +
> +    AVDictionary *pending_metadata;
>   } DecodeContext;
>   
>   static DecodeContext *decode_ctx(AVCodecInternal *avci)
> @@ -729,6 +731,8 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>   {
>       AVCodecInternal *avci = avctx->internal;
>       DecodeContext     *dc = decode_ctx(avci);
> +    const uint8_t *side_metadata;
> +    size_t size;
>       int ret;
>   
>       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> @@ -746,6 +750,14 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>           ret = av_packet_ref(avci->buffer_pkt, avpkt);
>           if (ret < 0)
>               return ret;
> +
> +        side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size);
> +        if (side_metadata) {
> +            av_dict_free(&dc->pending_metadata);
> +            ret = av_packet_unpack_dictionary(side_metadata, size, &dc->pending_metadata);
> +            if (ret < 0)
> +                return ret;
> +        }
>       } else
>           dc->draining_started = 1;
>   
> @@ -815,6 +827,7 @@ fail:
>   int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
>   {
>       AVCodecInternal *avci = avctx->internal;
> +    DecodeContext     *dc = decode_ctx(avci);
>       int ret;
>   
>       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> @@ -887,6 +900,12 @@ int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
>           }
>       }
>   #endif
> +
> +    if (dc->pending_metadata) {
> +        av_dict_copy(&frame->metadata, dc->pending_metadata, AV_DICT_APPEND);
> +        av_dict_free(&dc->pending_metadata);
> +    }
> +
>       return 0;
>   fail:
>       av_frame_unref(frame);
> @@ -2314,4 +2333,5 @@ void ff_decode_internal_uninit(AVCodecContext *avctx)
>       DecodeContext *dc = decode_ctx(avci);
>   
>       av_refstruct_unref(&dc->lcevc);
> +    av_dict_free(&dc->pending_metadata);
>   }

btw, jamrial mentioned that AVSTREAM_EVENT_FLAG_METADATA_UPDATED exists.
I only see it used to inform API users that metadata has been updated, 
but you should set the flag.

Other than that, I'm fine with the patchset. You should resend it with 
the change, and if there are no comments, I'll merge it in a few days.

Thanks

[-- 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 v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Romain Beauxis]

Le mar. 18 févr. 2025 à 17:28, Lynne <dev@lynne.ee> a écrit :
>
> On 17/02/2025 17:19, Romain Beauxis wrote:
> > libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
> > attach them to the next decoded frame.
> >
> > The metadata needs to be cached because there is no guarantee that each packet
> > will generate a decoded frame immediately.
> >
> > `AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
> > so regression risks seem low.
> >
> > This generic mechanism could be reused to support more in-band metadata update
> > in the future.
> >
> > ---
> >   libavcodec/decode.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index cac7e620d2..96e2f0ce95 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -97,6 +97,8 @@ typedef struct DecodeContext {
> >       int lcevc_frame;
> >       int width;
> >       int height;
> > +
> > +    AVDictionary *pending_metadata;
> >   } DecodeContext;
> >
> >   static DecodeContext *decode_ctx(AVCodecInternal *avci)
> > @@ -729,6 +731,8 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >   {
> >       AVCodecInternal *avci = avctx->internal;
> >       DecodeContext     *dc = decode_ctx(avci);
> > +    const uint8_t *side_metadata;
> > +    size_t size;
> >       int ret;
> >
> >       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > @@ -746,6 +750,14 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >           ret = av_packet_ref(avci->buffer_pkt, avpkt);
> >           if (ret < 0)
> >               return ret;
> > +
> > +        side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size);
> > +        if (side_metadata) {
> > +            av_dict_free(&dc->pending_metadata);
> > +            ret = av_packet_unpack_dictionary(side_metadata, size, &dc->pending_metadata);
> > +            if (ret < 0)
> > +                return ret;
> > +        }
> >       } else
> >           dc->draining_started = 1;
> >
> > @@ -815,6 +827,7 @@ fail:
> >   int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> >   {
> >       AVCodecInternal *avci = avctx->internal;
> > +    DecodeContext     *dc = decode_ctx(avci);
> >       int ret;
> >
> >       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > @@ -887,6 +900,12 @@ int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> >           }
> >       }
> >   #endif
> > +
> > +    if (dc->pending_metadata) {
> > +        av_dict_copy(&frame->metadata, dc->pending_metadata, AV_DICT_APPEND);
> > +        av_dict_free(&dc->pending_metadata);
> > +    }
> > +
> >       return 0;
> >   fail:
> >       av_frame_unref(frame);
> > @@ -2314,4 +2333,5 @@ void ff_decode_internal_uninit(AVCodecContext *avctx)
> >       DecodeContext *dc = decode_ctx(avci);
> >
> >       av_refstruct_unref(&dc->lcevc);
> > +    av_dict_free(&dc->pending_metadata);
> >   }
>
> btw, jamrial mentioned that AVSTREAM_EVENT_FLAG_METADATA_UPDATED exists.
> I only see it used to inform API users that metadata has been updated,
> but you should set the flag.
>
> Other than that, I'm fine with the patchset. You should resend it with
> the change, and if there are no comments, I'll merge it in a few days.

Thank you for the review.

Thanks for pointing that out. Since ogg vorbis already supports
AVSTREAM_EVENT_FLAG_METADATA_UPDATED I think it would make sense to
favor our the whole process from there.

However, I have some questions about how this is implemented in vorbis.

The vorbis implementation:
* Parses new comment metadata and adds them to the AVStream's metadata
* Entries are appended in this case there is already an existing one:

         if (av_dict_get(*m, t, NULL, 0))
            av_dict_set(m, t, ";", AV_DICT_APPEND);
        av_dict_set(m, t, v, AV_DICT_APPEND);

* Then AVSTREAM_EVENT_FLAG_METADATA_UPDATED is set on the AVStream.

I can see a couple of issues:
* This is pretty unexpected behavior. Any consumer of the stream's
metadata would expect a single title, not an accumulation of all
previously parsed titles
* This is also a memory leak.

Should we make it so the stream's duplicated metadata are taking the
last value instead?

If so, I'm happy to include this change in the patchset and submit an
updated one which uses the same factored out mechanism in vorbis, flac
and opus.

-- 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 v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Romain Beauxis]

Le mar. 18 févr. 2025 à 22:11, Romain Beauxis
<romain.beauxis@gmail.com> a écrit :
>
> Le mar. 18 févr. 2025 à 17:28, Lynne <dev@lynne.ee> a écrit :
> >
> > On 17/02/2025 17:19, Romain Beauxis wrote:
> > > libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
> > > attach them to the next decoded frame.
> > >
> > > The metadata needs to be cached because there is no guarantee that each packet
> > > will generate a decoded frame immediately.
> > >
> > > `AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
> > > so regression risks seem low.
> > >
> > > This generic mechanism could be reused to support more in-band metadata update
> > > in the future.
> > >
> > > ---
> > >   libavcodec/decode.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > >
> > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > > index cac7e620d2..96e2f0ce95 100644
> > > --- a/libavcodec/decode.c
> > > +++ b/libavcodec/decode.c
> > > @@ -97,6 +97,8 @@ typedef struct DecodeContext {
> > >       int lcevc_frame;
> > >       int width;
> > >       int height;
> > > +
> > > +    AVDictionary *pending_metadata;
> > >   } DecodeContext;
> > >
> > >   static DecodeContext *decode_ctx(AVCodecInternal *avci)
> > > @@ -729,6 +731,8 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> > >   {
> > >       AVCodecInternal *avci = avctx->internal;
> > >       DecodeContext     *dc = decode_ctx(avci);
> > > +    const uint8_t *side_metadata;
> > > +    size_t size;
> > >       int ret;
> > >
> > >       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > > @@ -746,6 +750,14 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> > >           ret = av_packet_ref(avci->buffer_pkt, avpkt);
> > >           if (ret < 0)
> > >               return ret;
> > > +
> > > +        side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size);
> > > +        if (side_metadata) {
> > > +            av_dict_free(&dc->pending_metadata);
> > > +            ret = av_packet_unpack_dictionary(side_metadata, size, &dc->pending_metadata);
> > > +            if (ret < 0)
> > > +                return ret;
> > > +        }
> > >       } else
> > >           dc->draining_started = 1;
> > >
> > > @@ -815,6 +827,7 @@ fail:
> > >   int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> > >   {
> > >       AVCodecInternal *avci = avctx->internal;
> > > +    DecodeContext     *dc = decode_ctx(avci);
> > >       int ret;
> > >
> > >       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > > @@ -887,6 +900,12 @@ int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> > >           }
> > >       }
> > >   #endif
> > > +
> > > +    if (dc->pending_metadata) {
> > > +        av_dict_copy(&frame->metadata, dc->pending_metadata, AV_DICT_APPEND);
> > > +        av_dict_free(&dc->pending_metadata);
> > > +    }
> > > +
> > >       return 0;
> > >   fail:
> > >       av_frame_unref(frame);
> > > @@ -2314,4 +2333,5 @@ void ff_decode_internal_uninit(AVCodecContext *avctx)
> > >       DecodeContext *dc = decode_ctx(avci);
> > >
> > >       av_refstruct_unref(&dc->lcevc);
> > > +    av_dict_free(&dc->pending_metadata);
> > >   }
> >
> > btw, jamrial mentioned that AVSTREAM_EVENT_FLAG_METADATA_UPDATED exists.
> > I only see it used to inform API users that metadata has been updated,
> > but you should set the flag.
> >
> > Other than that, I'm fine with the patchset. You should resend it with
> > the change, and if there are no comments, I'll merge it in a few days.
>
> Thank you for the review.
>
> Thanks for pointing that out. Since ogg vorbis already supports
> AVSTREAM_EVENT_FLAG_METADATA_UPDATED I think it would make sense to
> favor our the whole process from there.
>
> However, I have some questions about how this is implemented in vorbis.
>
> The vorbis implementation:
> * Parses new comment metadata and adds them to the AVStream's metadata
> * Entries are appended in this case there is already an existing one:
>
>          if (av_dict_get(*m, t, NULL, 0))
>             av_dict_set(m, t, ";", AV_DICT_APPEND);
>         av_dict_set(m, t, v, AV_DICT_APPEND);
>
> * Then AVSTREAM_EVENT_FLAG_METADATA_UPDATED is set on the AVStream.
>
> I can see a couple of issues:
> * This is pretty unexpected behavior. Any consumer of the stream's
> metadata would expect a single title, not an accumulation of all
> previously parsed titles
> * This is also a memory leak.
>
> Should we make it so the stream's duplicated metadata are taking the
> last value instead?

Actually, I'd miss a sneaky `av_dict_free` call!

Updated patchset coming soon!

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