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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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