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

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

Le sam. 31 mai 2025 à 15:08, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> a écrit :
>
> Romain Beauxis:
> > Le ven. 30 mai 2025 à 19:44, Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> a écrit :
> >>
> >> Romain Beauxis:
> >>> ---
> >>>  libavcodec/vorbis_parser.h                 | 11 ++++
> >>>  libavcodec/vorbisdec.c                     | 75 +++++++++++++---------
> >>>  libavformat/oggparsevorbis.c               | 67 ++++++++++++++++++-
> >>>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 -
> >>>  tests/ref/fate/trac-2739.txt               |  4 +-
> >>>  5 files changed, 121 insertions(+), 39 deletions(-)
> >>>
> >>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> >>> index 789932ac49..b176fe536c 100644
> >>> --- a/libavcodec/vorbis_parser.h
> >>> +++ b/libavcodec/vorbis_parser.h
> >>> @@ -30,6 +30,17 @@
> >>>
> >>>  typedef struct AVVorbisParseContext AVVorbisParseContext;
> >>>
> >>> +/**
> >>> + * Used by the vorbis parser to pass new chained stream headers
> >>> + * as extradata.
> >>> + */
> >>> +typedef struct vorbis_new_extradata {
> >>> +    uint8_t *header;
> >>> +    size_t   header_size;
> >>> +    uint8_t *setup;
> >>> +    size_t   setup_size;
> >>> +} vorbis_new_extradata;
> >>> +
> >>>  /**
> >>>   * Allocate and initialize the Vorbis parser using headers in the extradata.
> >>>   */
> >>> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> >>> index adbd726183..a4b159ba9b 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,47 +1779,59 @@ 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;
> >>> +    vorbis_new_extradata *new_extradata;
> >>> +    const uint8_t *header;
> >>> +    const uint8_t *setup;
> >>>
> >>>      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)
> >>> -            return ret;
> >>> +    new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
> >>> +        avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
> >>>
> >>> -        vorbis_free(vc);
> >>> -        if ((ret = vorbis_parse_id_hdr(vc))) {
> >>> -            av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
> >>> -            vorbis_free(vc);
> >>> -            return ret;
> >>> -        }
> >>> +    if (new_extradata) {
> >>> +        header = new_extradata->header;
> >>> +        setup = new_extradata->setup;
> >>>
> >>> -        av_channel_layout_uninit(&avctx->ch_layout);
> >>> -        if (vc->audio_channels > 8) {
> >>> -            avctx->ch_layout.order       = AV_CHANNEL_ORDER_UNSPEC;
> >>> -            avctx->ch_layout.nb_channels = vc->audio_channels;
> >>> -        } else {
> >>> -            av_channel_layout_copy(&avctx->ch_layout, &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
> >>> -        }
> >>> +        if (new_extradata->header_size > 7 && *header == 1) {
> >>> +            if ((ret = init_get_bits8(
> >>> +                            gb, header + 1,
> >>> +                            new_extradata->header_size - 1)) < 0)
> >>> +                return ret;
> >>>
> >>> -        avctx->sample_rate = vc->audio_samplerate;
> >>> -        return buf_size;
> >>> -    }
> >>> +            vorbis_free(vc);
> >>> +            if ((ret = vorbis_parse_id_hdr(vc))) {
> >>> +                av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
> >>> +                vorbis_free(vc);
> >>> +                return ret;
> >>> +            }
> >>>
> >>> -    if (*buf == 3 && buf_size > 7) {
> >>> -        av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> >>> -        return buf_size;
> >>> -    }
> >>> +            av_channel_layout_uninit(&avctx->ch_layout);
> >>> +            if (vc->audio_channels > 8) {
> >>> +                avctx->ch_layout.order       = AV_CHANNEL_ORDER_UNSPEC;
> >>> +                avctx->ch_layout.nb_channels = vc->audio_channels;
> >>> +            } else {
> >>> +                av_channel_layout_copy(
> >>> +                    &avctx->ch_layout,
> >>> +                    &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
> >>> +            }
> >>>
> >>> -    if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> >>> -        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> >>> -            return ret;
> >>> +            avctx->sample_rate = vc->audio_samplerate;
> >>> +        }
> >>>
> >>> -        if ((ret = vorbis_parse_setup_hdr(vc))) {
> >>> -            av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> >>> -            vorbis_free(vc);
> >>> -            return ret;
> >>> +        if (new_extradata->setup_size > 7 && *setup == 5 &&
> >>> +            vc->channel_residues && !vc->modes) {
> >>> +            if ((ret = init_get_bits8(
> >>> +                           gb, setup + 1,
> >>> +                           new_extradata->setup_size - 1)) < 0)
> >>> +                return ret;
> >>> +
> >>> +            if ((ret = vorbis_parse_setup_hdr(vc))) {
> >>> +                av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> >>> +                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..f8e66e8127 100644
> >>> --- a/libavformat/oggparsevorbis.c
> >>> +++ b/libavformat/oggparsevorbis.c
> >>> @@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
> >>>      struct ogg *ogg = s->priv_data;
> >>>      struct ogg_stream *os = ogg->streams + idx;
> >>>      struct oggvorbis_private *priv = os->private;
> >>> +    vorbis_new_extradata *new_extradata;
> >>>      int i;
> >>>      if (os->private) {
> >>>          av_vorbis_parse_free(&priv->vp);
> >>>          for (i = 0; i < 3; i++)
> >>>              av_freep(&priv->packet[i]);
> >>>      }
> >>> +
> >>> +    if (os->new_extradata) {
> >>> +        new_extradata = (vorbis_new_extradata *)os->new_extradata;
> >>> +        av_freep(&new_extradata->header);
> >>> +        av_freep(&new_extradata->setup);
> >>> +    }
> >>>  }
> >>>
> >>>  static int vorbis_update_metadata(AVFormatContext *s, int idx)
> >>> @@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> >>>      struct ogg *ogg = s->priv_data;
> >>>      struct ogg_stream *os = ogg->streams + idx;
> >>>      struct oggvorbis_private *priv = os->private;
> >>> +    vorbis_new_extradata *new_extradata;
> >>>      int duration, flags = 0;
> >>> +    int skip_packet = 0;
> >>> +    int ret;
> >>>
> >>>      if (!priv->vp)
> >>>          return AVERROR_INVALIDDATA;
> >>> @@ -496,10 +506,61 @@ 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;
> >>> +
> >>> +            if (!os->new_extradata) {
> >>> +                os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> >>> +                if (!os->new_extradata)
> >>> +                    return AVERROR(ENOMEM);
> >>> +            }
> >>> +
> >>> +            os->new_extradata_size = sizeof(vorbis_new_extradata);
> >>> +            new_extradata = (vorbis_new_extradata *)os->new_extradata;
> >>> +
> >>> +            ret = av_reallocp(&new_extradata->header, os->psize);
> >>> +            if (ret < 0)
> >>> +                return ret;
> >>> +
> >>> +            memcpy(new_extradata->header,  os->buf + os->pstart, os->psize);
> >>> +            new_extradata->header_size = os->psize;
> >>> +
> >>> +            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) {
> >>> +            if (!os->new_extradata) {
> >>> +                os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> >>> +                if (!os->new_extradata)
> >>> +                    return AVERROR(ENOMEM);
> >>> +            }
> >>> +
> >>> +            os->new_extradata_size = sizeof(vorbis_new_extradata);
> >>> +            new_extradata = (vorbis_new_extradata *)os->new_extradata;
> >>> +
> >>> +            ret = av_reallocp(&new_extradata->setup, os->psize);
> >>> +            if (ret < 0)
> >>> +                return ret;
> >>> +
> >>> +            memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
> >>> +            new_extradata->setup_size = os->psize;
> >>> +
> >>> +            skip_packet = 1;
> >>>          }
> >>> +
> >>>          os->pduration = duration;
> >>>      }
> >>>
> >>> @@ -521,7 +582,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 = {
> >>
> >> There are multiple issues with this patch:
> >
> > Thank you for your feedback.
> >
> >> 1. The side data structures are not padded, leading to
> >> heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
> >
> > Do you have a pointer to this issue? Is there a failing test here:
> > https://fate.ffmpeg.org/ ?
> >
>
> I noted it when I ran FATE with (Clang-)ASAN locally. Seems like none of
> the ASAN/valgrind fate boxes tested your commit.
>
> >> 2. The side data structures are not flat and therefore not suitable for
> >> use as AVPacketSideData. (The setup and header arrays are currently
> >> owned by the demuxer, yet an AVPacket is supposed to be valid on its
> >> own. But this side data becomes invalid when the demuxer encounters a
> >> new side data (and reallocates its internal buffers) or when the demuxer
> >> is closed.)
> >
> > I can work on that.
>
> Actually, thinking about this a bit more: New extradata via side data
> should use the same format as ordinary extradata, so there is no need to
> add a new struct and APIchanges for that.

Just sent an updated patch, let me know what you think!

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