[FFmpeg-devel] [PATCH] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[FFmpeg-devel] [PATCH] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
memory storage for the extradata.

PR review comments addressed:
* Use flat memory array
* Rename structure to follow convention
* Add stddef.h header

Bonus: libavcodec/vorbisdec.c diff is greatly improved!

---
 libavcodec/vorbis_parser.h                 |  7 +++
 libavcodec/vorbisdec.c                     | 45 ++++++++++++-----
 libavformat/oggparsevorbis.c               | 56 ++++++++++++++++++++--
 tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 --
 4 files changed, 94 insertions(+), 17 deletions(-)

diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
index 789932ac49..9010723590 100644
--- a/libavcodec/vorbis_parser.h
+++ b/libavcodec/vorbis_parser.h
@@ -27,6 +27,7 @@
 #define AVCODEC_VORBIS_PARSER_H
 
 #include <stdint.h>
+#include <stddef.h>
 
 typedef struct AVVorbisParseContext AVVorbisParseContext;
 
@@ -45,6 +46,12 @@ void av_vorbis_parse_free(AVVorbisParseContext **s);
 #define VORBIS_FLAG_COMMENT 0x00000002
 #define VORBIS_FLAG_SETUP   0x00000004
 
+typedef struct AVVorbisHeaderExtradata {
+    unsigned int header_type;
+    size_t   header_size;
+    uint8_t header[];
+} AVVorbisHeaderExtradata;
+
 /**
  * Get the duration for a Vorbis packet.
  *
diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
index adbd726183..177d3c95c2 100644
--- a/libavcodec/vorbisdec.c
+++ b/libavcodec/vorbisdec.c
@@ -43,6 +43,7 @@
 #include "vorbis.h"
 #include "vorbisdsp.h"
 #include "vorbis_data.h"
+#include "vorbis_parser.h"
 #include "xiph.h"
 
 #define V_NB_BITS 8
@@ -1778,11 +1779,40 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
     GetBitContext *gb = &vc->gb;
     float *channel_ptrs[255];
     int i, len, ret;
+    size_t new_extradata_size;
+    const uint8_t *new_extradata;
+    AVVorbisHeaderExtradata *new_header;
+    const uint8_t *header = NULL;
+    size_t header_size = 0;
+    const uint8_t *setup = NULL;
+    size_t setup_size = 0;
 
     ff_dlog(NULL, "packet length %d \n", buf_size);
 
-    if (*buf == 1 && buf_size > 7) {
-        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
+    new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
+                        &new_extradata_size);
+
+    if (new_extradata) {
+        i = 0;
+        while (i < new_extradata_size) {
+            new_header = (AVVorbisHeaderExtradata *)(new_extradata+i);
+
+            if (new_header->header_type == VORBIS_FLAG_HEADER) {
+                header_size = new_header->header_size;
+                header = new_header->header;
+            }
+
+            if (new_header->header_type == VORBIS_FLAG_SETUP) {
+                setup_size = new_header->header_size;
+                setup = new_header->header;
+            }
+
+            i += sizeof(AVVorbisHeaderExtradata)+new_header->header_size;
+        }
+    }
+
+    if (header_size > 7 && *header == 1) {
+        if ((ret = init_get_bits8(gb, header + 1, header_size - 1)) < 0)
             return ret;
 
         vorbis_free(vc);
@@ -1801,16 +1831,10 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
         }
 
         avctx->sample_rate = vc->audio_samplerate;
-        return buf_size;
-    }
-
-    if (*buf == 3 && buf_size > 7) {
-        av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
-        return buf_size;
     }
 
-    if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
-        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
+    if (setup_size > 7 && *setup == 5 && vc->channel_residues && !vc->modes) {
+        if ((ret = init_get_bits8(gb, setup + 1, setup_size - 1)) < 0)
             return ret;
 
         if ((ret = vorbis_parse_setup_hdr(vc))) {
@@ -1818,7 +1842,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
             vorbis_free(vc);
             return ret;
         }
-        return buf_size;
     }
 
     if (!vc->channel_residues || !vc->modes) {
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 62cc2da6de..7e812846fe 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -434,6 +434,9 @@ 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;
+    int ret, header_size;
+    AVVorbisHeaderExtradata *new_header;
 
     if (!priv->vp)
         return AVERROR_INVALIDDATA;
@@ -496,10 +499,57 @@ 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) {
-            vorbis_update_metadata(s, idx);
+        }
+
+        if (flags & VORBIS_FLAG_HEADER) {
+            ret = vorbis_parse_header(s, s->streams[idx], os->buf + os->pstart, os->psize);
+            if (ret < 0)
+                return ret;
+
+            header_size = sizeof(AVVorbisHeaderExtradata)+os->psize;
+            ret = av_reallocp(&os->new_extradata,
+                os->new_extradata_size+header_size);
+
+            if (ret < 0)
+                return ret;
+
+            new_header = (AVVorbisHeaderExtradata *)(os->new_extradata+os->new_extradata_size);
+            new_header->header_type = VORBIS_FLAG_HEADER;
+            new_header->header_size = os->psize;
+            memcpy(new_header->header, os->buf + os->pstart, os->psize);
+
+            os->new_extradata_size += header_size;
+
+            skip_packet = 1;
+        }
+
+        if (flags & VORBIS_FLAG_COMMENT) {
+            ret = vorbis_update_metadata(s, idx);
+            if (ret < 0)
+                return ret;
+
             flags = 0;
+            skip_packet = 1;
+        }
+
+        if (flags & VORBIS_FLAG_SETUP) {
+            header_size = sizeof(AVVorbisHeaderExtradata)+os->psize;
+            ret = av_reallocp(&os->new_extradata,
+                os->new_extradata_size+header_size);
+
+            if (ret < 0)
+                return ret;
+
+            new_header = (AVVorbisHeaderExtradata *)(os->new_extradata+os->new_extradata_size);
+            new_header->header_type = VORBIS_FLAG_SETUP;
+            new_header->header_size = os->psize;
+            memcpy(new_header->header, os->buf + os->pstart, os->psize);
+
+            os->new_extradata_size += header_size;
+
+            skip_packet = 1;
         }
+
         os->pduration = duration;
     }
 
@@ -521,7 +571,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".

Re: [FFmpeg-devel] [PATCH] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Andreas Rheinhardt]

Romain Beauxis:
> This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
> memory storage for the extradata.
> 
> PR review comments addressed:
> * Use flat memory array
> * Rename structure to follow convention
> * Add stddef.h header
> 
> Bonus: libavcodec/vorbisdec.c diff is greatly improved!
> 
> ---
>  libavcodec/vorbis_parser.h                 |  7 +++
>  libavcodec/vorbisdec.c                     | 45 ++++++++++++-----
>  libavformat/oggparsevorbis.c               | 56 ++++++++++++++++++++--
>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 --
>  4 files changed, 94 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> index 789932ac49..9010723590 100644
> --- a/libavcodec/vorbis_parser.h
> +++ b/libavcodec/vorbis_parser.h
> @@ -27,6 +27,7 @@
>  #define AVCODEC_VORBIS_PARSER_H
>  
>  #include <stdint.h>
> +#include <stddef.h>
>  
>  typedef struct AVVorbisParseContext AVVorbisParseContext;
>  
> @@ -45,6 +46,12 @@ void av_vorbis_parse_free(AVVorbisParseContext **s);
>  #define VORBIS_FLAG_COMMENT 0x00000002
>  #define VORBIS_FLAG_SETUP   0x00000004
>  
> +typedef struct AVVorbisHeaderExtradata {
> +    unsigned int header_type;
> +    size_t   header_size;
> +    uint8_t header[];
> +} AVVorbisHeaderExtradata;
> +

It seems you started this work before my latest mail
https://ffmpeg.org/pipermail/ffmpeg-devel/2025-May/344422.html:
AV_PKT_DATA_NEW_EXTRADATA should use the same format as ordinary
extradata. (A user may e.g. use this new extradata for muxing, where the
ordinary extradata is expected.) This generally allows to reuse
extradata parsing functions (potentially after having factored them out
of the init function).

Besides that, there are some problems with your approach: You are simply
casting a pointer into the new extradata to AVVorbisHeaderExtradata*,
although the address may not be properly aligned. You are also not
checking that the extradata contains as much data as header_size claims
it does. The layout of your new extradata also depends on the system
(due to sizeof(header_size) and due to endianness), which makes it hard
to checksum it. And you forgot the APIchanges entry/version bump for
this struct (this point will become moot in a future version of this
patch, when no new struct is added).

>  /**
>   * Get the duration for a Vorbis packet.
>   *
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index adbd726183..177d3c95c2 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -43,6 +43,7 @@
>  #include "vorbis.h"
>  #include "vorbisdsp.h"
>  #include "vorbis_data.h"
> +#include "vorbis_parser.h"
>  #include "xiph.h"
>  
>  #define V_NB_BITS 8
> @@ -1778,11 +1779,40 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>      GetBitContext *gb = &vc->gb;
>      float *channel_ptrs[255];
>      int i, len, ret;
> +    size_t new_extradata_size;
> +    const uint8_t *new_extradata;
> +    AVVorbisHeaderExtradata *new_header;
> +    const uint8_t *header = NULL;
> +    size_t header_size = 0;
> +    const uint8_t *setup = NULL;
> +    size_t setup_size = 0;
>  
>      ff_dlog(NULL, "packet length %d \n", buf_size);
>  
> -    if (*buf == 1 && buf_size > 7) {
> -        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> +    new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> +                        &new_extradata_size);
> +
> +    if (new_extradata) {
> +        i = 0;
> +        while (i < new_extradata_size) {
> +            new_header = (AVVorbisHeaderExtradata *)(new_extradata+i);
> +
> +            if (new_header->header_type == VORBIS_FLAG_HEADER) {
> +                header_size = new_header->header_size;
> +                header = new_header->header;
> +            }
> +
> +            if (new_header->header_type == VORBIS_FLAG_SETUP) {
> +                setup_size = new_header->header_size;
> +                setup = new_header->header;
> +            }
> +
> +            i += sizeof(AVVorbisHeaderExtradata)+new_header->header_size;
> +        }
> +    }
> +
> +    if (header_size > 7 && *header == 1) {
> +        if ((ret = init_get_bits8(gb, header + 1, header_size - 1)) < 0)
>              return ret;
>  
>          vorbis_free(vc);
> @@ -1801,16 +1831,10 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>          }
>  
>          avctx->sample_rate = vc->audio_samplerate;
> -        return buf_size;
> -    }
> -
> -    if (*buf == 3 && buf_size > 7) {
> -        av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> -        return buf_size;
>      }
>  
> -    if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> -        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> +    if (setup_size > 7 && *setup == 5 && vc->channel_residues && !vc->modes) {
> +        if ((ret = init_get_bits8(gb, setup + 1, setup_size - 1)) < 0)
>              return ret;
>  
>          if ((ret = vorbis_parse_setup_hdr(vc))) {
> @@ -1818,7 +1842,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>              vorbis_free(vc);
>              return ret;
>          }
> -        return buf_size;
>      }
>  
>      if (!vc->channel_residues || !vc->modes) {
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 62cc2da6de..7e812846fe 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -434,6 +434,9 @@ 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;
> +    int ret, header_size;
> +    AVVorbisHeaderExtradata *new_header;
>  
>      if (!priv->vp)
>          return AVERROR_INVALIDDATA;
> @@ -496,10 +499,57 @@ 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) {
> -            vorbis_update_metadata(s, idx);
> +        }
> +
> +        if (flags & VORBIS_FLAG_HEADER) {
> +            ret = vorbis_parse_header(s, s->streams[idx], os->buf + os->pstart, os->psize);
> +            if (ret < 0)
> +                return ret;
> +
> +            header_size = sizeof(AVVorbisHeaderExtradata)+os->psize;
> +            ret = av_reallocp(&os->new_extradata,
> +                os->new_extradata_size+header_size);
> +
> +            if (ret < 0)
> +                return ret;
> +
> +            new_header = (AVVorbisHeaderExtradata *)(os->new_extradata+os->new_extradata_size);
> +            new_header->header_type = VORBIS_FLAG_HEADER;
> +            new_header->header_size = os->psize;
> +            memcpy(new_header->header, os->buf + os->pstart, os->psize);
> +
> +            os->new_extradata_size += header_size;
> +
> +            skip_packet = 1;
> +        }
> +
> +        if (flags & VORBIS_FLAG_COMMENT) {
> +            ret = vorbis_update_metadata(s, idx);
> +            if (ret < 0)
> +                return ret;
> +
>              flags = 0;
> +            skip_packet = 1;
> +        }
> +
> +        if (flags & VORBIS_FLAG_SETUP) {
> +            header_size = sizeof(AVVorbisHeaderExtradata)+os->psize;
> +            ret = av_reallocp(&os->new_extradata,
> +                os->new_extradata_size+header_size);
> +
> +            if (ret < 0)
> +                return ret;
> +
> +            new_header = (AVVorbisHeaderExtradata *)(os->new_extradata+os->new_extradata_size);
> +            new_header->header_type = VORBIS_FLAG_SETUP;
> +            new_header->header_size = os->psize;
> +            memcpy(new_header->header, os->buf + os->pstart, os->psize);
> +
> +            os->new_extradata_size += header_size;
> +
> +            skip_packet = 1;
>          }
> +
>          os->pduration = duration;
>      }
>  
> @@ -521,7 +571,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

_______________________________________________
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] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

Le mar. 3 juin 2025 à 12:12, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> a écrit :
>
> Romain Beauxis:
> > This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
> > memory storage for the extradata.
> >
> > PR review comments addressed:
> > * Use flat memory array
> > * Rename structure to follow convention
> > * Add stddef.h header
> >
> > Bonus: libavcodec/vorbisdec.c diff is greatly improved!
> >
> > ---
> >  libavcodec/vorbis_parser.h                 |  7 +++
> >  libavcodec/vorbisdec.c                     | 45 ++++++++++++-----
> >  libavformat/oggparsevorbis.c               | 56 ++++++++++++++++++++--
> >  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 --
> >  4 files changed, 94 insertions(+), 17 deletions(-)
> >
> > diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> > index 789932ac49..9010723590 100644
> > --- a/libavcodec/vorbis_parser.h
> > +++ b/libavcodec/vorbis_parser.h
> > @@ -27,6 +27,7 @@
> >  #define AVCODEC_VORBIS_PARSER_H
> >
> >  #include <stdint.h>
> > +#include <stddef.h>
> >
> >  typedef struct AVVorbisParseContext AVVorbisParseContext;
> >
> > @@ -45,6 +46,12 @@ void av_vorbis_parse_free(AVVorbisParseContext **s);
> >  #define VORBIS_FLAG_COMMENT 0x00000002
> >  #define VORBIS_FLAG_SETUP   0x00000004
> >
> > +typedef struct AVVorbisHeaderExtradata {
> > +    unsigned int header_type;
> > +    size_t   header_size;
> > +    uint8_t header[];
> > +} AVVorbisHeaderExtradata;
> > +
>
> It seems you started this work before my latest mail
> https://ffmpeg.org/pipermail/ffmpeg-devel/2025-May/344422.html:
> AV_PKT_DATA_NEW_EXTRADATA should use the same format as ordinary
> extradata. (A user may e.g. use this new extradata for muxing, where the
> ordinary extradata is expected.) This generally allows to reuse
> extradata parsing functions (potentially after having factored them out
> of the init function).

I'm not sure what you mean. Could you elaborate?

If you look at https://ffmpeg.org/pipermail/ffmpeg-devel/2025-June/344446.html
you will see that the format seems like ordinary extradata to me. In
fact, as soon as the key is changed, vorbis metadata update finally
starts to flow as expected without changing the decoder side of
things, as exemplified by the test diff in the changeset.

> Besides that, there are some problems with your approach: You are simply
> casting a pointer into the new extradata to AVVorbisHeaderExtradata*,
> although the address may not be properly aligned. You are also not
> checking that the extradata contains as much data as header_size claims
> it does. The layout of your new extradata also depends on the system
> (due to sizeof(header_size) and due to endianness), which makes it hard
> to checksum it. And you forgot the APIchanges entry/version bump for
> this struct (this point will become moot in a future version of this
> patch, when no new struct is added).

Ok this makes sense, I can work on it.

Let me recap the needs:
* extradata needs to be a flat memory array
* extradata needs to be platform-independent and constantly checksum-able.
* What's the exact requirement for alignment?
Considering that the array contains 2 or more sequentially stored
header chunks, do you require the start of each header chunk to have
to be aligned? Is there a technical reason for that?

-- Romain


> >  /**
> >   * Get the duration for a Vorbis packet.
> >   *
> > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> > index adbd726183..177d3c95c2 100644
> > --- a/libavcodec/vorbisdec.c
> > +++ b/libavcodec/vorbisdec.c
> > @@ -43,6 +43,7 @@
> >  #include "vorbis.h"
> >  #include "vorbisdsp.h"
> >  #include "vorbis_data.h"
> > +#include "vorbis_parser.h"
> >  #include "xiph.h"
> >
> >  #define V_NB_BITS 8
> > @@ -1778,11 +1779,40 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> >      GetBitContext *gb = &vc->gb;
> >      float *channel_ptrs[255];
> >      int i, len, ret;
> > +    size_t new_extradata_size;
> > +    const uint8_t *new_extradata;
> > +    AVVorbisHeaderExtradata *new_header;
> > +    const uint8_t *header = NULL;
> > +    size_t header_size = 0;
> > +    const uint8_t *setup = NULL;
> > +    size_t setup_size = 0;
> >
> >      ff_dlog(NULL, "packet length %d \n", buf_size);
> >
> > -    if (*buf == 1 && buf_size > 7) {
> > -        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > +    new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> > +                        &new_extradata_size);
> > +
> > +    if (new_extradata) {
> > +        i = 0;
> > +        while (i < new_extradata_size) {
> > +            new_header = (AVVorbisHeaderExtradata *)(new_extradata+i);
> > +
> > +            if (new_header->header_type == VORBIS_FLAG_HEADER) {
> > +                header_size = new_header->header_size;
> > +                header = new_header->header;
> > +            }
> > +
> > +            if (new_header->header_type == VORBIS_FLAG_SETUP) {
> > +                setup_size = new_header->header_size;
> > +                setup = new_header->header;
> > +            }
> > +
> > +            i += sizeof(AVVorbisHeaderExtradata)+new_header->header_size;
> > +        }
> > +    }
> > +
> > +    if (header_size > 7 && *header == 1) {
> > +        if ((ret = init_get_bits8(gb, header + 1, header_size - 1)) < 0)
> >              return ret;
> >
> >          vorbis_free(vc);
> > @@ -1801,16 +1831,10 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> >          }
> >
> >          avctx->sample_rate = vc->audio_samplerate;
> > -        return buf_size;
> > -    }
> > -
> > -    if (*buf == 3 && buf_size > 7) {
> > -        av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> > -        return buf_size;
> >      }
> >
> > -    if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> > -        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > +    if (setup_size > 7 && *setup == 5 && vc->channel_residues && !vc->modes) {
> > +        if ((ret = init_get_bits8(gb, setup + 1, setup_size - 1)) < 0)
> >              return ret;
> >
> >          if ((ret = vorbis_parse_setup_hdr(vc))) {
> > @@ -1818,7 +1842,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> >              vorbis_free(vc);
> >              return ret;
> >          }
> > -        return buf_size;
> >      }
> >
> >      if (!vc->channel_residues || !vc->modes) {
> > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > index 62cc2da6de..7e812846fe 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -434,6 +434,9 @@ 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;
> > +    int ret, header_size;
> > +    AVVorbisHeaderExtradata *new_header;
> >
> >      if (!priv->vp)
> >          return AVERROR_INVALIDDATA;
> > @@ -496,10 +499,57 @@ 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) {
> > -            vorbis_update_metadata(s, idx);
> > +        }
> > +
> > +        if (flags & VORBIS_FLAG_HEADER) {
> > +            ret = vorbis_parse_header(s, s->streams[idx], os->buf + os->pstart, os->psize);
> > +            if (ret < 0)
> > +                return ret;
> > +
> > +            header_size = sizeof(AVVorbisHeaderExtradata)+os->psize;
> > +            ret = av_reallocp(&os->new_extradata,
> > +                os->new_extradata_size+header_size);
> > +
> > +            if (ret < 0)
> > +                return ret;
> > +
> > +            new_header = (AVVorbisHeaderExtradata *)(os->new_extradata+os->new_extradata_size);
> > +            new_header->header_type = VORBIS_FLAG_HEADER;
> > +            new_header->header_size = os->psize;
> > +            memcpy(new_header->header, os->buf + os->pstart, os->psize);
> > +
> > +            os->new_extradata_size += header_size;
> > +
> > +            skip_packet = 1;
> > +        }
> > +
> > +        if (flags & VORBIS_FLAG_COMMENT) {
> > +            ret = vorbis_update_metadata(s, idx);
> > +            if (ret < 0)
> > +                return ret;
> > +
> >              flags = 0;
> > +            skip_packet = 1;
> > +        }
> > +
> > +        if (flags & VORBIS_FLAG_SETUP) {
> > +            header_size = sizeof(AVVorbisHeaderExtradata)+os->psize;
> > +            ret = av_reallocp(&os->new_extradata,
> > +                os->new_extradata_size+header_size);
> > +
> > +            if (ret < 0)
> > +                return ret;
> > +
> > +            new_header = (AVVorbisHeaderExtradata *)(os->new_extradata+os->new_extradata_size);
> > +            new_header->header_type = VORBIS_FLAG_SETUP;
> > +            new_header->header_size = os->psize;
> > +            memcpy(new_header->header, os->buf + os->pstart, os->psize);
> > +
> > +            os->new_extradata_size += header_size;
> > +
> > +            skip_packet = 1;
> >          }
> > +
> >          os->pduration = duration;
> >      }
> >
> > @@ -521,7 +571,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
>
_______________________________________________
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] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Andreas Rheinhardt]

Romain Beauxis:
> Le mar. 3 juin 2025 à 12:12, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> a écrit :
>>
>> Romain Beauxis:
>>> This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
>>> memory storage for the extradata.
>>>
>>> PR review comments addressed:
>>> * Use flat memory array
>>> * Rename structure to follow convention
>>> * Add stddef.h header
>>>
>>> Bonus: libavcodec/vorbisdec.c diff is greatly improved!
>>>
>>> ---
>>>  libavcodec/vorbis_parser.h                 |  7 +++
>>>  libavcodec/vorbisdec.c                     | 45 ++++++++++++-----
>>>  libavformat/oggparsevorbis.c               | 56 ++++++++++++++++++++--
>>>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 --
>>>  4 files changed, 94 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
>>> index 789932ac49..9010723590 100644
>>> --- a/libavcodec/vorbis_parser.h
>>> +++ b/libavcodec/vorbis_parser.h
>>> @@ -27,6 +27,7 @@
>>>  #define AVCODEC_VORBIS_PARSER_H
>>>
>>>  #include <stdint.h>
>>> +#include <stddef.h>
>>>
>>>  typedef struct AVVorbisParseContext AVVorbisParseContext;
>>>
>>> @@ -45,6 +46,12 @@ void av_vorbis_parse_free(AVVorbisParseContext **s);
>>>  #define VORBIS_FLAG_COMMENT 0x00000002
>>>  #define VORBIS_FLAG_SETUP   0x00000004
>>>
>>> +typedef struct AVVorbisHeaderExtradata {
>>> +    unsigned int header_type;
>>> +    size_t   header_size;
>>> +    uint8_t header[];
>>> +} AVVorbisHeaderExtradata;
>>> +
>>
>> It seems you started this work before my latest mail
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2025-May/344422.html:
>> AV_PKT_DATA_NEW_EXTRADATA should use the same format as ordinary
>> extradata. (A user may e.g. use this new extradata for muxing, where the
>> ordinary extradata is expected.) This generally allows to reuse
>> extradata parsing functions (potentially after having factored them out
>> of the init function).
> 
> I'm not sure what you mean. Could you elaborate?
> 
> If you look at https://ffmpeg.org/pipermail/ffmpeg-devel/2025-June/344446.html
> you will see that the format seems like ordinary extradata to me. In
> fact, as soon as the key is changed, vorbis metadata update finally
> starts to flow as expected without changing the decoder side of
> things, as exemplified by the test diff in the changeset.

I do not get what your link to the patch exchanging
AV_PKT_DATA_METADATA_UPDATE for AV_PKT_DATA_STRINGS_METADATA is supposed
to mean. The format of these two types of metadata is the same, but this
has nothing to do with AV_PKT_DATA_NEW_EXTRADATA or with extradata in
general.

> 
>> Besides that, there are some problems with your approach: You are simply
>> casting a pointer into the new extradata to AVVorbisHeaderExtradata*,
>> although the address may not be properly aligned. You are also not
>> checking that the extradata contains as much data as header_size claims
>> it does. The layout of your new extradata also depends on the system
>> (due to sizeof(header_size) and due to endianness), which makes it hard
>> to checksum it. And you forgot the APIchanges entry/version bump for
>> this struct (this point will become moot in a future version of this
>> patch, when no new struct is added).
> 
> Ok this makes sense, I can work on it.
> 
> Let me recap the needs:
> * extradata needs to be a flat memory array
> * extradata needs to be platform-independent and constantly checksum-able.
> * What's the exact requirement for alignment?
> Considering that the array contains 2 or more sequentially stored
> header chunks, do you require the start of each header chunk to have
> to be aligned? Is there a technical reason for that?

a) Reading or writing anything is undefined behavior unless the memory
is suitably aligned for the type used to access it; failure to do so
causes crashes (bus errors) on some systems.
b) As I said: Don't add a new structure for extradata, use the already
established format for vorbis.

- Andreas

_______________________________________________
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] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

Le mar. 3 juin 2025 à 14:20, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> a écrit :
>
> Romain Beauxis:
> > Le mar. 3 juin 2025 à 12:12, Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> a écrit :
> >>
> >> Romain Beauxis:
> >>> This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
> >>> memory storage for the extradata.
> >>>
> >>> PR review comments addressed:
> >>> * Use flat memory array
> >>> * Rename structure to follow convention
> >>> * Add stddef.h header
> >>>
> >>> Bonus: libavcodec/vorbisdec.c diff is greatly improved!
> >>>
> >>> ---
> >>>  libavcodec/vorbis_parser.h                 |  7 +++
> >>>  libavcodec/vorbisdec.c                     | 45 ++++++++++++-----
> >>>  libavformat/oggparsevorbis.c               | 56 ++++++++++++++++++++--
> >>>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 --
> >>>  4 files changed, 94 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> >>> index 789932ac49..9010723590 100644
> >>> --- a/libavcodec/vorbis_parser.h
> >>> +++ b/libavcodec/vorbis_parser.h
> >>> @@ -27,6 +27,7 @@
> >>>  #define AVCODEC_VORBIS_PARSER_H
> >>>
> >>>  #include <stdint.h>
> >>> +#include <stddef.h>
> >>>
> >>>  typedef struct AVVorbisParseContext AVVorbisParseContext;
> >>>
> >>> @@ -45,6 +46,12 @@ void av_vorbis_parse_free(AVVorbisParseContext **s);
> >>>  #define VORBIS_FLAG_COMMENT 0x00000002
> >>>  #define VORBIS_FLAG_SETUP   0x00000004
> >>>
> >>> +typedef struct AVVorbisHeaderExtradata {
> >>> +    unsigned int header_type;
> >>> +    size_t   header_size;
> >>> +    uint8_t header[];
> >>> +} AVVorbisHeaderExtradata;
> >>> +
> >>
> >> It seems you started this work before my latest mail
> >> https://ffmpeg.org/pipermail/ffmpeg-devel/2025-May/344422.html:
> >> AV_PKT_DATA_NEW_EXTRADATA should use the same format as ordinary
> >> extradata. (A user may e.g. use this new extradata for muxing, where the
> >> ordinary extradata is expected.) This generally allows to reuse
> >> extradata parsing functions (potentially after having factored them out
> >> of the init function).
> >
> > I'm not sure what you mean. Could you elaborate?
> >
> > If you look at https://ffmpeg.org/pipermail/ffmpeg-devel/2025-June/344446.html
> > you will see that the format seems like ordinary extradata to me. In
> > fact, as soon as the key is changed, vorbis metadata update finally
> > starts to flow as expected without changing the decoder side of
> > things, as exemplified by the test diff in the changeset.
>
> I do not get what your link to the patch exchanging
> AV_PKT_DATA_METADATA_UPDATE for AV_PKT_DATA_STRINGS_METADATA is supposed
> to mean. The format of these two types of metadata is the same, but this
> has nothing to do with AV_PKT_DATA_NEW_EXTRADATA or with extradata in
> general.

My bad.

> >
> >> Besides that, there are some problems with your approach: You are simply
> >> casting a pointer into the new extradata to AVVorbisHeaderExtradata*,
> >> although the address may not be properly aligned. You are also not
> >> checking that the extradata contains as much data as header_size claims
> >> it does. The layout of your new extradata also depends on the system
> >> (due to sizeof(header_size) and due to endianness), which makes it hard
> >> to checksum it. And you forgot the APIchanges entry/version bump for
> >> this struct (this point will become moot in a future version of this
> >> patch, when no new struct is added).
> >
> > Ok this makes sense, I can work on it.
> >
> > Let me recap the needs:
> > * extradata needs to be a flat memory array
> > * extradata needs to be platform-independent and constantly checksum-able.
> > * What's the exact requirement for alignment?
> > Considering that the array contains 2 or more sequentially stored
> > header chunks, do you require the start of each header chunk to have
> > to be aligned? Is there a technical reason for that?
>
> a) Reading or writing anything is undefined behavior unless the memory
> is suitably aligned for the type used to access it; failure to do so
> causes crashes (bus errors) on some systems.
> b) As I said: Don't add a new structure for extradata, use the already
> established format for vorbis.

The logic for vorbis is to have two header packets. Any of these 2
might be missing.

(There could also be extra packets but we can assume we'd drop the
last one of each type in this case and only pass at most two packets
at any given time).

The extradata then gets attached to the first data packet that comes
after those header packets.

Therefore, the data needs to be able to accommodate for any subset of
those two packets depending on the situation.

How do you suggest the established vorbis format should be used for
this use-case?

Thanks,
-- Romain
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".