[FFmpeg-devel] [PATCH v2 1/2] Parse ogg/flac header again after processing a new chained ogg bitstream.

[FFmpeg-devel] [PATCH v2 1/2] Parse ogg/flac header again after processing a new chained ogg bitstream.

[Romain Beauxis]

This patch makes sure that ogg/flac headers are parsed again when
encountering a new logic stream inside a chained ogg bistream[1].

This patches makes it possible to retrieve metadata in chained ogg/flac
bitstreams. It is particularly important because ogg/flac is one of the
only (if not the only one) lossless container supported over HTTP/icecast.

The patch has been tested with various ogg/flac encoders and appears to
work fine with ffmpeg.

Changes since last version:
* Make sure to clear the stream's metadata before parsing again.

1: https://xiph.org/ogg/doc/oggstream.html

---
 libavformat/oggdec.c       | 7 +++++--
 libavformat/oggparseflac.c | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 5339fdd32c..d986e19817 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -239,8 +239,11 @@ 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)
+    /* Parse opus and flac header on new chained bitstream.
+     * For opus, header contains required extradata as new packet
+     * For both formats, this makes it possible to read chained metadata. */
+    if (codec == &ff_opus_codec ||
+        codec == &ff_flac_codec)
         os->header = -1;
 
     return i;
diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
index f25ed9cc15..932907fa1a 100644
--- a/libavformat/oggparseflac.c
+++ b/libavformat/oggparseflac.c
@@ -72,6 +72,8 @@ flac_header (AVFormatContext * s, int idx)
 
         avpriv_set_pts_info(st, 64, 1, samplerate);
     } else if (mdt == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
+        /* New metadata packet; release old data. */
+        av_dict_free(&st->metadata);
         ff_vorbis_stream_comment(s, st, os->buf + os->pstart + 4, os->psize - 4);
     }
 
-- 
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 v1 2/2] Add stream dump test with test for ogg/flac.

[Romain Beauxis]

This patch adds a fate test for ogg/flac chained streams.

First, a new api-dump-stream-meta-test utility is introduced then it is
used to dump the metadata of a ogg/flac chained sample.

Sample is available here: https://www.dropbox.com/scl/fo/fxt2edwkyj2mjc9qubku5/AICHxJyxMMAK8MIJqWLcvk4?rlkey=mlt12lsu741ejukz0p5qtn9rq&dl=0

Sample is very short so the dump output is:

Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100 flac:title=First Stream
Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100 flac:title=First Stream
Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100 flac:title=Second Stream
Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100 flac:title=Second Stream

---
 tests/Makefile                        |  2 +
 tests/api/Makefile                    |  2 +-
 tests/api/api-dump-stream-meta-test.c | 88 +++++++++++++++++++++++++++
 tests/fate/api.mak                    |  5 ++
 tests/fate/ogg-flac.mak               | 11 ++++
 5 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 tests/api/api-dump-stream-meta-test.c
 create mode 100644 tests/fate/ogg-flac.mak

diff --git a/tests/Makefile b/tests/Makefile
index f9f5fc07f3..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
@@ -277,6 +278,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..d750e0aecb
--- /dev/null
+++ b/tests/api/api-dump-stream-meta-test.c
@@ -0,0 +1,88 @@
+/*
+ * 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 "libavformat/avformat.h"
+
+static int dump_stream_meta(const char *input_filename)
+{
+    AVPacket *pkt = NULL;
+    AVFormatContext *fmt_ctx = NULL;
+    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;
+    }
+
+    pkt = av_packet_alloc();
+    if (!pkt) {
+        av_log(NULL, AV_LOG_ERROR, "Cannot allocate packet\n");
+        result = AVERROR(ENOMEM);
+        goto end;
+    }
+
+    for (;;) {
+        result = av_read_frame(fmt_ctx, pkt);
+        if (result)
+            goto end;
+
+        result = av_dict_get_string(
+            fmt_ctx->streams[pkt->stream_index]->metadata, &metadata, '=', ':');
+        if (result < 0)
+            goto end;
+
+        printf("Stream ID: %d, PTS: %lld, DTS: %lld, metadata: %s\n",
+            pkt->stream_index, pkt->pts, pkt->dts, metadata);
+        av_free(metadata);
+        av_packet_unref(pkt);
+    }
+
+end:
+    av_packet_free(&pkt);
+    avformat_close_input(&fmt_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;
+}
diff --git a/tests/fate/api.mak b/tests/fate/api.mak
index d2868e57ac..a9872a5589 100644
--- a/tests/fate/api.mak
+++ b/tests/fate/api.mak
@@ -1,3 +1,8 @@
+FATE_API_LIBAVFORMAT-$(call ENCDEC, FLAC, FLAC) += api-dump-stream-meta
+fate-api-ogg-flac-meta: $(APITESTSDIR)/api-dump-stream-meta-test$(EXESUF)
+fate-api-ogg-flac-meta: CMD = run $(APITESTSDIR)/api-flac-test$(EXESUF)
+fate-api-ogg-flac-meta: CMP = null
+
 FATE_API_LIBAVCODEC-$(call ENCDEC, FLAC, FLAC) += fate-api-flac
 fate-api-flac: $(APITESTSDIR)/api-flac-test$(EXESUF)
 fate-api-flac: CMD = run $(APITESTSDIR)/api-flac-test$(EXESUF)
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 v1 2/2] Add stream dump test with test for ogg/flac.

[Romain Beauxis]

Le mer. 29 janv. 2025 à 15:43, Romain Beauxis
<romain.beauxis@gmail.com> a écrit :
>
> This patch adds a fate test for ogg/flac chained streams.
>
> First, a new api-dump-stream-meta-test utility is introduced then it is
> used to dump the metadata of a ogg/flac chained sample.
>
> Sample is available here:
https://www.dropbox.com/scl/fo/fxt2edwkyj2mjc9qubku5/AICHxJyxMMAK8MIJqWLcvk4?rlkey=mlt12lsu741ejukz0p5qtn9rq&dl=0
>
> Sample is very short so the dump output is:
>
> Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
flac:title=First Stream
> Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100
flac:title=First Stream
> Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
flac:title=Second Stream
> Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100
flac:title=Second Stream

I forgot to add the output of the stream meta dump prior to the changes:

Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
flac:title=First Stream
Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100
flac:title=First Stream
Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
flac:title=First Stream
Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
flac:title=First Stream
Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
flac:title=First Stream
Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100
flac:title=First Stream

The extra two packets are the header packets.

The PTS/DTS discontinuity is already present, the decoder seems to
have no problem with them.

I'd be happy to do a follow-up to tentatively adjust PTS/DTS if that'd
make sense/is possible.


Le mer. 29 janv. 2025 à 15:43, Romain Beauxis <romain.beauxis@gmail.com> a
écrit :

> This patch adds a fate test for ogg/flac chained streams.
>
> First, a new api-dump-stream-meta-test utility is introduced then it is
> used to dump the metadata of a ogg/flac chained sample.
>
> Sample is available here:
> https://www.dropbox.com/scl/fo/fxt2edwkyj2mjc9qubku5/AICHxJyxMMAK8MIJqWLcvk4?rlkey=mlt12lsu741ejukz0p5qtn9rq&dl=0
>
> Sample is very short so the dump output is:
>
> Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
> flac:title=First Stream
> Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100
> flac:title=First Stream
> Stream ID: 0, PTS: 0, DTS: 0, metadata: encoder=Lavc61.19.100
> flac:title=Second Stream
> Stream ID: 0, PTS: 4608, DTS: 4608, metadata: encoder=Lavc61.19.100
> flac:title=Second Stream
>
> ---
>  tests/Makefile                        |  2 +
>  tests/api/Makefile                    |  2 +-
>  tests/api/api-dump-stream-meta-test.c | 88 +++++++++++++++++++++++++++
>  tests/fate/api.mak                    |  5 ++
>  tests/fate/ogg-flac.mak               | 11 ++++
>  5 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 tests/api/api-dump-stream-meta-test.c
>  create mode 100644 tests/fate/ogg-flac.mak
>
> diff --git a/tests/Makefile b/tests/Makefile
> index f9f5fc07f3..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
> @@ -277,6 +278,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..d750e0aecb
> --- /dev/null
> +++ b/tests/api/api-dump-stream-meta-test.c
> @@ -0,0 +1,88 @@
> +/*
> + * 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 "libavformat/avformat.h"
> +
> +static int dump_stream_meta(const char *input_filename)
> +{
> +    AVPacket *pkt = NULL;
> +    AVFormatContext *fmt_ctx = NULL;
> +    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;
> +    }
> +
> +    pkt = av_packet_alloc();
> +    if (!pkt) {
> +        av_log(NULL, AV_LOG_ERROR, "Cannot allocate packet\n");
> +        result = AVERROR(ENOMEM);
> +        goto end;
> +    }
> +
> +    for (;;) {
> +        result = av_read_frame(fmt_ctx, pkt);
> +        if (result)
> +            goto end;
> +
> +        result = av_dict_get_string(
> +            fmt_ctx->streams[pkt->stream_index]->metadata, &metadata,
> '=', ':');
> +        if (result < 0)
> +            goto end;
> +
> +        printf("Stream ID: %d, PTS: %lld, DTS: %lld, metadata: %s\n",
> +            pkt->stream_index, pkt->pts, pkt->dts, metadata);
> +        av_free(metadata);
> +        av_packet_unref(pkt);
> +    }
> +
> +end:
> +    av_packet_free(&pkt);
> +    avformat_close_input(&fmt_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;
> +}
> diff --git a/tests/fate/api.mak b/tests/fate/api.mak
> index d2868e57ac..a9872a5589 100644
> --- a/tests/fate/api.mak
> +++ b/tests/fate/api.mak
> @@ -1,3 +1,8 @@
> +FATE_API_LIBAVFORMAT-$(call ENCDEC, FLAC, FLAC) += api-dump-stream-meta
> +fate-api-ogg-flac-meta: $(APITESTSDIR)/api-dump-stream-meta-test$(EXESUF)
> +fate-api-ogg-flac-meta: CMD = run $(APITESTSDIR)/api-flac-test$(EXESUF)
> +fate-api-ogg-flac-meta: CMP = null
> +
>  FATE_API_LIBAVCODEC-$(call ENCDEC, FLAC, FLAC) += fate-api-flac
>  fate-api-flac: $(APITESTSDIR)/api-flac-test$(EXESUF)
>  fate-api-flac: CMD = run $(APITESTSDIR)/api-flac-test$(EXESUF)
> 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 v2 1/2] Parse ogg/flac header again after processing a new chained ogg bitstream.

[Marvin Scholz]

On 29 Jan 2025, at 15:40, Romain Beauxis wrote:

> This patch makes sure that ogg/flac headers are parsed again when
> encountering a new logic stream inside a chained ogg bistream[1].
>
> This patches makes it possible to retrieve metadata in chained ogg/flac
> bitstreams. It is particularly important because ogg/flac is one of the
> only (if not the only one) lossless container supported over HTTP/icecast.
>
> The patch has been tested with various ogg/flac encoders and appears to
> work fine with ffmpeg.
>
> Changes since last version:
> * Make sure to clear the stream's metadata before parsing again.
>
> 1: https://xiph.org/ogg/doc/oggstream.html
>

Hi, thanks a lot for trying to solve this, see remarks inline below:

> ---
>  libavformat/oggdec.c       | 7 +++++--
>  libavformat/oggparseflac.c | 2 ++
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 5339fdd32c..d986e19817 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -239,8 +239,11 @@ 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)
> +    /* Parse opus and flac header on new chained bitstream.
> +     * For opus, header contains required extradata as new packet
> +     * For both formats, this makes it possible to read chained metadata. */
> +    if (codec == &ff_opus_codec ||
> +        codec == &ff_flac_codec)
>          os->header = -1;
>
>      return i;
> diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> index f25ed9cc15..932907fa1a 100644
> --- a/libavformat/oggparseflac.c
> +++ b/libavformat/oggparseflac.c
> @@ -72,6 +72,8 @@ flac_header (AVFormatContext * s, int idx)
>
>          avpriv_set_pts_info(st, 64, 1, samplerate);
>      } else if (mdt == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
> +        /* New metadata packet; release old data. */
> +        av_dict_free(&st->metadata);

I do not think it’s fine to change this mid-demuxing, as the caller
has no way to know this changed „behind its back“ so may still hold
on to the old pointer and run into invalid memory access.

I checked with James and he suggested that maybe propagating the new
metadata with AVPacket’s side_data might be a better approach, similar
to AV_PKT_DATA_NEW_EXTRADATA.

>          ff_vorbis_stream_comment(s, st, os->buf + os->pstart + 4, os->psize - 4);
>      }
>
> -- 
> 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 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 v2 1/2] Parse ogg/flac header again after processing a new chained ogg bitstream.

[Romain Beauxis]

Le mer. 29 janv. 2025 à 17:40, Marvin Scholz <epirat07@gmail.com> a écrit :
>
>
>
> On 29 Jan 2025, at 15:40, Romain Beauxis wrote:
>
> > This patch makes sure that ogg/flac headers are parsed again when
> > encountering a new logic stream inside a chained ogg bistream[1].
> >
> > This patches makes it possible to retrieve metadata in chained ogg/flac
> > bitstreams. It is particularly important because ogg/flac is one of the
> > only (if not the only one) lossless container supported over
HTTP/icecast.
> >
> > The patch has been tested with various ogg/flac encoders and appears to
> > work fine with ffmpeg.
> >
> > Changes since last version:
> > * Make sure to clear the stream's metadata before parsing again.
> >
> > 1: https://xiph.org/ogg/doc/oggstream.html
> >
>
> Hi, thanks a lot for trying to solve this, see remarks inline below:
>
> > ---
> >  libavformat/oggdec.c       | 7 +++++--
> >  libavformat/oggparseflac.c | 2 ++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > index 5339fdd32c..d986e19817 100644
> > --- a/libavformat/oggdec.c
> > +++ b/libavformat/oggdec.c
> > @@ -239,8 +239,11 @@ 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)
> > +    /* Parse opus and flac header on new chained bitstream.
> > +     * For opus, header contains required extradata as new packet
> > +     * For both formats, this makes it possible to read chained
metadata. */
> > +    if (codec == &ff_opus_codec ||
> > +        codec == &ff_flac_codec)
> >          os->header = -1;
> >
> >      return i;
> > diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> > index f25ed9cc15..932907fa1a 100644
> > --- a/libavformat/oggparseflac.c
> > +++ b/libavformat/oggparseflac.c
> > @@ -72,6 +72,8 @@ flac_header (AVFormatContext * s, int idx)
> >
> >          avpriv_set_pts_info(st, 64, 1, samplerate);
> >      } else if (mdt == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
> > +        /* New metadata packet; release old data. */
> > +        av_dict_free(&st->metadata);
>
> I do not think it’s fine to change this mid-demuxing, as the caller
> has no way to know this changed „behind its back“ so may still hold
> on to the old pointer and run into invalid memory access.
>
> I checked with James and he suggested that maybe propagating the new
> metadata with AVPacket’s side_data might be a better approach, similar
> to AV_PKT_DATA_NEW_EXTRADATA.

Thank you for the review!

Do you mean the st->metadata dictionary?

The nature of `av_dict` is to be an ever-changing pointer; each time a
value is changed, the underlying pointer is changed so I think keeping a
long-term pointer on it is a programming error.

It's also worth noting that this is the same method used currently by the
vorbis decoder.

That being said, updating the AVStream is not natural and requires the user
to constantly check on it.

It would be more natural to have:
* The headers packets flow out of the demuxer as it is now so anyone
operating at this level can see them and act accordingly.
* Have the decoded frame carry the new metadata, which is a more natural
place for it.

I'll look into this. Hopefully this is a better approach and also one that
could be generalized to all ogg-encapsulated formats..

-- 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 v2 1/2] Parse ogg/flac header again after processing a new chained ogg bitstream.

[Romain Beauxis]

Le jeu. 30 janv. 2025 à 08:08, Romain Beauxis <romain.beauxis@gmail.com> a
écrit :
>
>
>
>
> Le mer. 29 janv. 2025 à 17:40, Marvin Scholz <epirat07@gmail.com> a écrit
:
> >
> >
> >
> > On 29 Jan 2025, at 15:40, Romain Beauxis wrote:
> >
> > > This patch makes sure that ogg/flac headers are parsed again when
> > > encountering a new logic stream inside a chained ogg bistream[1].
> > >
> > > This patches makes it possible to retrieve metadata in chained
ogg/flac
> > > bitstreams. It is particularly important because ogg/flac is one of
the
> > > only (if not the only one) lossless container supported over
HTTP/icecast.
> > >
> > > The patch has been tested with various ogg/flac encoders and appears
to
> > > work fine with ffmpeg.
> > >
> > > Changes since last version:
> > > * Make sure to clear the stream's metadata before parsing again.
> > >
> > > 1: https://xiph.org/ogg/doc/oggstream.html
> > >
> >
> > Hi, thanks a lot for trying to solve this, see remarks inline below:
> >
> > > ---
> > >  libavformat/oggdec.c       | 7 +++++--
> > >  libavformat/oggparseflac.c | 2 ++
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > index 5339fdd32c..d986e19817 100644
> > > --- a/libavformat/oggdec.c
> > > +++ b/libavformat/oggdec.c
> > > @@ -239,8 +239,11 @@ 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)
> > > +    /* Parse opus and flac header on new chained bitstream.
> > > +     * For opus, header contains required extradata as new packet
> > > +     * For both formats, this makes it possible to read chained
metadata. */
> > > +    if (codec == &ff_opus_codec ||
> > > +        codec == &ff_flac_codec)
> > >          os->header = -1;
> > >
> > >      return i;
> > > diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> > > index f25ed9cc15..932907fa1a 100644
> > > --- a/libavformat/oggparseflac.c
> > > +++ b/libavformat/oggparseflac.c
> > > @@ -72,6 +72,8 @@ flac_header (AVFormatContext * s, int idx)
> > >
> > >          avpriv_set_pts_info(st, 64, 1, samplerate);
> > >      } else if (mdt == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
> > > +        /* New metadata packet; release old data. */
> > > +        av_dict_free(&st->metadata);
> >
> > I do not think it’s fine to change this mid-demuxing, as the caller
> > has no way to know this changed „behind its back“ so may still hold
> > on to the old pointer and run into invalid memory access.
> >
> > I checked with James and he suggested that maybe propagating the new
> > metadata with AVPacket’s side_data might be a better approach, similar
> > to AV_PKT_DATA_NEW_EXTRADATA.
>
> Thank you for the review!
>
> Do you mean the st->metadata dictionary?
>
> The nature of `av_dict` is to be an ever-changing pointer; each time a
value is changed, the underlying pointer is changed so I think keeping a
long-term pointer on it is a programming error.
>
> It's also worth noting that this is the same method used currently by the
vorbis decoder.
>
> That being said, updating the AVStream is not natural and requires the
user to constantly check on it.
>
> It would be more natural to have:
> * The headers packets flow out of the demuxer as it is now so anyone
operating at this level can see them and act accordingly.
> * Have the decoded frame carry the new metadata, which is a more natural
place for it.
>
> I'll look into this. Hopefully this is a better approach and also one
that could be generalized to all ogg-encapsulated formats..

I'm sending another version of the patch. It pushes the metadata decoding
to the codec decoding.

This makes sense from separation of concern perspective: packets keep
seeing the full encoded data and decoded frames see the decoded metadata.

However, this adds a dependency on libavformat to libavcodec.

I also looked at moving the vorbis comment parsing to libavutil but there's
a lot of metadata definitions and utils that at located in libavformat at
the moment so perhaps it makes sense this way.

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