[FFmpeg-devel] [PATCH v4 0/6] Remove chained ogg stream header packets from the demuxer

[FFmpeg-devel] [PATCH v4 0/6] Remove chained ogg stream header packets from the demuxer

[Romain Beauxis]

## Changes since last revision:
* Changed split into several more atomic patches as recommended.

Romain Beauxis (6):
  libavformat/oggdec.h: Document packet function return value.
  libavformat/oggdec.{c,h}: Implement packet skip on packet return value
    of 1
  ogg/opus: implement header packet skip in chained ogg bitstreams.
  ogg/flac: implement header packet skip in chained ogg bitstreams.
  ogg/vorbis: implement header packet skip in chained ogg bitstreams.
  libavformat/oggdec.h: Change paket function documentation to return 1
    on header packets only.

 libavformat/oggdec.c                       | 26 ++++++++++----------
 libavformat/oggdec.h                       |  7 ++++++
 libavformat/oggparseflac.c                 | 28 ++++++++++++++++++++--
 libavformat/oggparseopus.c                 | 11 +++++++++
 libavformat/oggparsevorbis.c               | 11 +++++++--
 tests/ref/fate/ogg-flac-chained-meta.txt   |  2 --
 tests/ref/fate/ogg-opus-chained-meta.txt   |  1 -
 tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 ---
 8 files changed, 67 insertions(+), 22 deletions(-)

-- 
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] libavformat/oggdec.h: Document packet function return value.

[Romain Beauxis]

---
 libavformat/oggdec.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index 43df23f4cb..5225b77a07 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -38,6 +38,12 @@ struct ogg_codec {
      *         -1 if an error occurred or for unsupported stream
      */
     int (*header)(AVFormatContext *, int);
+    /**
+     * Attempt to process a packet as a data packet
+     * @return < 0 (AVERROR) code or -1 on error
+     *         == 0 if the packet was a regular data packet.
+     *         == 0 or 1 if the packet was a header from a chained bitstream.
+     */
     int (*packet)(AVFormatContext *, int);
     /**
      * Translate a granule into a timestamp.
-- 
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] libavformat/oggdec.{c, h}: Implement packet skip on packet return value of 1

[Romain Beauxis]

---
 libavformat/oggdec.c | 22 ++++++++++++++--------
 libavformat/oggdec.h |  1 +
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 5339fdd32c..9baf8040a9 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -605,20 +605,26 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
     } else {
         os->pflags    = 0;
         os->pduration = 0;
+
+        ret = 0;
         if (os->codec && os->codec->packet) {
             if ((ret = os->codec->packet(s, idx)) < 0) {
                 av_log(s, AV_LOG_ERROR, "Packet processing failed: %s\n", av_err2str(ret));
                 return ret;
             }
         }
-        if (sid)
-            *sid = idx;
-        if (dstart)
-            *dstart = os->pstart;
-        if (dsize)
-            *dsize = os->psize;
-        if (fpos)
-            *fpos = os->sync_pos;
+
+        if (!ret) {
+            if (sid)
+                *sid = idx;
+            if (dstart)
+                *dstart = os->pstart;
+            if (dsize)
+                *dsize = os->psize;
+            if (fpos)
+                *fpos = os->sync_pos;
+        }
+
         os->pstart  += os->psize;
         os->psize    = 0;
         if(os->pstart == os->bufpos)
diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index 5225b77a07..bc670d0f1e 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -43,6 +43,7 @@ struct ogg_codec {
      * @return < 0 (AVERROR) code or -1 on error
      *         == 0 if the packet was a regular data packet.
      *         == 0 or 1 if the packet was a header from a chained bitstream.
+     *           (1 will cause the packet to be skiped in calling code (ogg_packet())
      */
     int (*packet)(AVFormatContext *, int);
     /**
-- 
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] ogg/opus: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

---
 libavformat/oggdec.c                     |  4 ----
 libavformat/oggparseopus.c               | 11 +++++++++++
 tests/ref/fate/ogg-opus-chained-meta.txt |  1 -
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 9baf8040a9..5557eb4a14 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..f54a9711df 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 1;
+    }
+
+    if (os->psize > 8 && !memcmp(packet, "OpusTags", 8))
+        return 1;
+
     if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS)) {
         int seg, d;
         int duration;
diff --git a/tests/ref/fate/ogg-opus-chained-meta.txt b/tests/ref/fate/ogg-opus-chained-meta.txt
index fc84b8b703..addc41c1eb 100644
--- a/tests/ref/fate/ogg-opus-chained-meta.txt
+++ b/tests/ref/fate/ogg-opus-chained-meta.txt
@@ -13,7 +13,6 @@ Stream ID: 0, frame PTS: 3528, metadata: N/A
 Stream ID: 0, packet PTS: 4488, packet DTS: 4488
 Stream ID: 0, frame PTS: 4488, metadata: N/A
 Stream ID: 0, packet PTS: -312, packet DTS: -312
-Stream ID: 0, new metadata: encoder=Lavc61.19.100 libopus;Lavc61.19.100 libopus:title=First Stream;Second Stream
 Stream ID: 0, frame PTS: -312, metadata: N/A
 Stream ID: 0, packet PTS: 648, packet DTS: 648
 Stream ID: 0, frame PTS: 648, metadata: N/A
-- 
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] ogg/flac: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

---
 libavformat/oggparseflac.c               | 28 ++++++++++++++++++++++--
 tests/ref/fate/ogg-flac-chained-meta.txt |  2 --
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
index f25ed9cc15..d66b85b09e 100644
--- a/libavformat/oggparseflac.c
+++ b/libavformat/oggparseflac.c
@@ -27,6 +27,8 @@
 #include "oggdec.h"
 
 #define OGG_FLAC_METADATA_TYPE_STREAMINFO 0x7F
+#define OGG_FLAC_MAGIC "\177FLAC"
+#define OGG_FLAC_MAGIC_SIZE sizeof(OGG_FLAC_MAGIC)-1
 
 static int
 flac_header (AVFormatContext * s, int idx)
@@ -78,6 +80,27 @@ 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;
+
+    if (os->psize > OGG_FLAC_MAGIC_SIZE &&
+        !memcmp(
+            os->buf + os->pstart,
+            OGG_FLAC_MAGIC,
+            OGG_FLAC_MAGIC_SIZE))
+        return 1;
+
+    if (os->psize > 0 &&
+        ((os->buf[os->pstart] & 0x7F) == FLAC_METADATA_TYPE_VORBIS_COMMENT)) {
+        return 1;
+    }
+
+    return 0;
+}
+
 static int
 old_flac_header (AVFormatContext * s, int idx)
 {
@@ -127,10 +150,11 @@ fail:
 }
 
 const struct ogg_codec ff_flac_codec = {
-    .magic = "\177FLAC",
-    .magicsize = 5,
+    .magic = OGG_FLAC_MAGIC,
+    .magicsize = OGG_FLAC_MAGIC_SIZE,
     .header = flac_header,
     .nb_header = 2,
+    .packet = flac_packet,
 };
 
 const struct ogg_codec ff_old_flac_codec = {
diff --git a/tests/ref/fate/ogg-flac-chained-meta.txt b/tests/ref/fate/ogg-flac-chained-meta.txt
index ad20ba935f..28e22aa29e 100644
--- a/tests/ref/fate/ogg-flac-chained-meta.txt
+++ b/tests/ref/fate/ogg-flac-chained-meta.txt
@@ -5,8 +5,6 @@ Stream ID: 0, frame PTS: 0, metadata: N/A
 Stream ID: 0, packet PTS: 4608, packet DTS: 4608
 Stream ID: 0, frame PTS: 4608, metadata: N/A
 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: N/A
 Stream ID: 0, packet PTS: 4608, packet DTS: 4608
 Stream ID: 0, frame PTS: 4608, metadata: N/A
-- 
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] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

---
 libavformat/oggparsevorbis.c               | 11 +++++++++--
 tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 ---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 9f50ab9ffc..8b4ae872d2 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -418,6 +418,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
     struct ogg_stream *os = ogg->streams + idx;
     struct oggvorbis_private *priv = os->private;
     int duration, flags = 0;
+    int skip_packet = 0;
 
     if (!priv->vp)
         return AVERROR_INVALIDDATA;
@@ -480,7 +481,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
         if (duration < 0) {
             os->pflags |= AV_PKT_FLAG_CORRUPT;
             return 0;
-        } else if (flags & VORBIS_FLAG_COMMENT) {
+        }
+
+        if (flags &
+            (VORBIS_FLAG_HEADER | VORBIS_FLAG_COMMENT | VORBIS_FLAG_SETUP))
+            skip_packet = 1;
+
+        if (flags & VORBIS_FLAG_COMMENT) {
             vorbis_update_metadata(s, idx);
             flags = 0;
         }
@@ -505,7 +512,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
         priv->final_duration += os->pduration;
     }
 
-    return 0;
+    return skip_packet;
 }
 
 const struct ogg_codec ff_vorbis_codec = {
diff --git a/tests/ref/fate/ogg-vorbis-chained-meta.txt b/tests/ref/fate/ogg-vorbis-chained-meta.txt
index b7a97c90e2..1206f86c1f 100644
--- a/tests/ref/fate/ogg-vorbis-chained-meta.txt
+++ b/tests/ref/fate/ogg-vorbis-chained-meta.txt
@@ -6,10 +6,7 @@ Stream ID: 0, frame PTS: 128, metadata: N/A
 Stream ID: 0, packet PTS: 704, packet DTS: 704
 Stream ID: 0, frame PTS: 704, metadata: N/A
 Stream ID: 0, packet PTS: 0, packet DTS: 0
-Stream ID: 0, packet PTS: 0, packet DTS: 0
 Stream ID: 0, new metadata: encoder=Lavc61.19.100 libvorbis:title=Second Stream
-Stream ID: 0, packet PTS: 0, packet DTS: 0
-Stream ID: 0, packet PTS: 0, packet DTS: 0
 Stream ID: 0, frame PTS: 0, metadata: N/A
 Stream ID: 0, packet PTS: 128, packet DTS: 128
 Stream ID: 0, frame PTS: 128, metadata: N/A
-- 
2.39.5 (Apple Git-154)

_______________________________________________
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] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only.

[Romain Beauxis]

---
 libavformat/oggdec.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index bc670d0f1e..3ac5adcbab 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -42,8 +42,8 @@ struct ogg_codec {
      * Attempt to process a packet as a data packet
      * @return < 0 (AVERROR) code or -1 on error
      *         == 0 if the packet was a regular data packet.
-     *         == 0 or 1 if the packet was a header from a chained bitstream.
-     *           (1 will cause the packet to be skiped in calling code (ogg_packet())
+     *         == 1 if the packet was a header from a chained bitstream.
+     *            This will cause the packet to be skiped in calling code (ogg_packet()
      */
     int (*packet)(AVFormatContext *, int);
     /**
-- 
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 5/6] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1745 bytes --]

On Tue, May 06, 2025 at 09:19:31AM -0500, Romain Beauxis wrote:
> ---
>  libavformat/oggparsevorbis.c               | 11 +++++++++--
>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 ---
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 9f50ab9ffc..8b4ae872d2 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -418,6 +418,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>      struct ogg_stream *os = ogg->streams + idx;
>      struct oggvorbis_private *priv = os->private;
>      int duration, flags = 0;
> +    int skip_packet = 0;
>  
>      if (!priv->vp)
>          return AVERROR_INVALIDDATA;
> @@ -480,7 +481,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          if (duration < 0) {
>              os->pflags |= AV_PKT_FLAG_CORRUPT;
>              return 0;
> -        } else if (flags & VORBIS_FLAG_COMMENT) {
> +        }
> +
> +        if (flags &
> +            (VORBIS_FLAG_HEADER | VORBIS_FLAG_COMMENT | VORBIS_FLAG_SETUP))
> +            skip_packet = 1;

can the content of the header containing "global" tables for vorbis change?
if so what does this patch do to such a stream ?

(I mean is this breaking such streams? or do they not exist or did that never work?)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 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 5/6] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

Le mer. 7 mai 2025 à 08:14, Michael Niedermayer
<michael@niedermayer.cc> a écrit :
>
> On Tue, May 06, 2025 at 09:19:31AM -0500, Romain Beauxis wrote:
> > ---
> >  libavformat/oggparsevorbis.c               | 11 +++++++++--
> >  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 ---
> >  2 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > index 9f50ab9ffc..8b4ae872d2 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -418,6 +418,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> >      struct ogg_stream *os = ogg->streams + idx;
> >      struct oggvorbis_private *priv = os->private;
> >      int duration, flags = 0;
> > +    int skip_packet = 0;
> >
> >      if (!priv->vp)
> >          return AVERROR_INVALIDDATA;
> > @@ -480,7 +481,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> >          if (duration < 0) {
> >              os->pflags |= AV_PKT_FLAG_CORRUPT;
> >              return 0;
> > -        } else if (flags & VORBIS_FLAG_COMMENT) {
> > +        }
> > +
> > +        if (flags &
> > +            (VORBIS_FLAG_HEADER | VORBIS_FLAG_COMMENT | VORBIS_FLAG_SETUP))
> > +            skip_packet = 1;
>
> can the content of the header containing "global" tables for vorbis change?
> if so what does this patch do to such a stream ?
>
> (I mean is this breaking such streams? or do they not exist or did that never work?)

Thanks for pointing this out.

It was always working in my tests but, upon further inspection, the
vorbis decoding code was doing something with those headers packets.

I'm working on an updated patchset taking care of this by passing the
setup packet (last packet) as extradata and processing it in the
decoder to mimic what's currently done.

I'm also gonna take this opportunity to add some safeguard in the
vorbis and opus code to make sure we fail when a new stream starts
with different channels count or sample rate.

Generally speaking, I'm intending the changes to be on best effort
w.r.t. to what's already working/not working but I think those two
changes will add and are not too hard to put in.

I'm gonna take some time to clean and test and will re-submit.

Thanks,
-- romain

> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you drop bombs on a foreign country and kill a hundred thousand
> innocent people, expect your government to call the consequence
> "unprovoked inhuman terrorist attacks" and use it to justify dropping
> more bombs and killing more people. The technology changed, the idea is old.
> _______________________________________________
> 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".