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

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

Michael Niedermayer:
> non flat extradata is problematic and was missed by reviewers
> 
> Found-by: mkver and jamrial

Actual patch LGTM, but I'd rather prefer to have my actual name here.

> This reverts commit 574f634e49847e2225ee50013afebf0de03ef013.
> ---
>  libavcodec/vorbis_parser.h                 | 11 ----
>  libavcodec/vorbisdec.c                     | 75 +++++++++-------------
>  libavformat/oggparsevorbis.c               | 67 +------------------
>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 +
>  4 files changed, 37 insertions(+), 119 deletions(-)
> 
> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> index b176fe536c7..789932ac492 100644
> --- a/libavcodec/vorbis_parser.h
> +++ b/libavcodec/vorbis_parser.h
> @@ -30,17 +30,6 @@
>  
>  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 a4b159ba9b2..adbd7261832 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -43,7 +43,6 @@
>  #include "vorbis.h"
>  #include "vorbisdsp.h"
>  #include "vorbis_data.h"
> -#include "vorbis_parser.h"
>  #include "xiph.h"
>  
>  #define V_NB_BITS 8
> @@ -1779,59 +1778,47 @@ 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);
>  
> -    new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
> -        avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
> -
> -    if (new_extradata) {
> -        header = new_extradata->header;
> -        setup = new_extradata->setup;
> -
> -        if (new_extradata->header_size > 7 && *header == 1) {
> -            if ((ret = init_get_bits8(
> -                            gb, header + 1,
> -                            new_extradata->header_size - 1)) < 0)
> -                return ret;
> +    if (*buf == 1 && buf_size > 7) {
> +        if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> +            return ret;
>  
> +        vorbis_free(vc);
> +        if ((ret = vorbis_parse_id_hdr(vc))) {
> +            av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
>              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;
> -            }
> -
> -            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]);
> -            }
> +            return ret;
> +        }
>  
> -            avctx->sample_rate = vc->audio_samplerate;
> +        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->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;
> +        avctx->sample_rate = vc->audio_samplerate;
> +        return buf_size;
> +    }
>  
> -            if ((ret = vorbis_parse_setup_hdr(vc))) {
> -                av_log(avctx, AV_LOG_ERROR, "Setup 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;
> +    }
> +
> +    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;
> +
> +        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 f8e66e8127c..62cc2da6de7 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -255,19 +255,12 @@ 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)
> @@ -440,10 +433,7 @@ 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;
> @@ -506,61 +496,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          if (duration < 0) {
>              os->pflags |= AV_PKT_FLAG_CORRUPT;
>              return 0;
> -        }
> -
> -        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;
> -
> +        } else if (flags & VORBIS_FLAG_COMMENT) {
> +            vorbis_update_metadata(s, idx);
>              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;
>      }
>  
> @@ -582,7 +521,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          priv->final_duration += os->pduration;
>      }
>  
> -    return skip_packet;
> +    return 0;
>  }
>  
>  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 1206f86c1f3..b7a97c90e29 100644
> --- a/tests/ref/fate/ogg-vorbis-chained-meta.txt
> +++ b/tests/ref/fate/ogg-vorbis-chained-meta.txt
> @@ -6,7 +6,10 @@ 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".