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

[Romain Beauxis <romain.beauxis@gmail.com>]

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