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

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

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:
1. The side data structures are not padded, leading to
heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
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.)
3. The vorbis_new_extradata name does not abide by our naming
conventions; given that this is a public header, it would need an AV prefix.
4. You forgot to add an APIchanges entry and bump lavc minor for your
addition.
5. You forgot to add stddef.h to vorbis_parser.h.
6. You should be aware that our parsing API does not work with
AVPackets, but with raw buffers. It (or rather: parse_packet() in
libavformat/demux.c) therefore has a tendency to put the side data to
the wrong packets or completely omit it (this happens when the parser
introduces a delay). Vorbis-in-ogg does not enable the parser, so it is
(currently) unaffected by this.

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