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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 9084 bytes --]

On Fri, May 09, 2025 at 06:43:26PM -0500, Romain Beauxis wrote:
> ---
>  libavcodec/vorbisdec.c                     |  37 +----
>  libavformat/oggparsevorbis.c               | 174 +++++++++++++--------
>  tests/ref/fate/ogg-vorbis-chained-meta.txt |   3 -
>  3 files changed, 117 insertions(+), 97 deletions(-)
> 
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index a778dc6b58..f069ac6ab3 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -1776,39 +1776,17 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>      GetBitContext *gb = &vc->gb;
>      float *channel_ptrs[255];
>      int i, len, ret;
> +    const int8_t *new_extradata;
> +    size_t new_extradata_size;
>  
>      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;
> -
> -        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]);
> -        }
> -
> -        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;
> -    }
> +    new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> +                                            &new_extradata_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 (new_extradata && *new_extradata == 5 && new_extradata_size > 7 &&
> +        vc->channel_residues && !vc->modes) {
> +        if ((ret = init_get_bits8(gb, new_extradata + 1, new_extradata_size - 1)) < 0)
>              return ret;
>  
>          if ((ret = vorbis_parse_setup_hdr(vc))) {
> @@ -1816,7 +1794,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 9f50ab9ffc..452728b54d 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -293,6 +293,62 @@ static int vorbis_update_metadata(AVFormatContext *s, int idx)
>      return ret;
>  }
>  
> +static int vorbis_parse_header(AVFormatContext *s, AVStream *st,
> +                               const uint8_t *p, unsigned int psize)
> +{
> +    unsigned blocksize, bs0, bs1;
> +    int srate;
> +    int channels;
> +
> +    if (psize != 30)
> +        return AVERROR_INVALIDDATA;
> +
> +    p += 7; /* skip "\001vorbis" tag */
> +
> +    if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> +        return AVERROR_INVALIDDATA;
> +
> +    channels = bytestream_get_byte(&p);
> +    if (st->codecpar->ch_layout.nb_channels &&
> +        channels != st->codecpar->ch_layout.nb_channels) {
> +        av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> +        return AVERROR_PATCHWELCOME;
> +    }
> +    st->codecpar->ch_layout.nb_channels = channels;
> +    srate               = bytestream_get_le32(&p);
> +    p += 4; // skip maximum bitrate
> +    st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal bitrate
> +    p += 4; // skip minimum bitrate
> +
> +    blocksize = bytestream_get_byte(&p);
> +    bs0       = blocksize & 15;
> +    bs1       = blocksize >> 4;
> +
> +    if (bs0 > bs1)
> +        return AVERROR_INVALIDDATA;
> +    if (bs0 < 6 || bs1 > 13)
> +        return AVERROR_INVALIDDATA;
> +
> +    if (bytestream_get_byte(&p) != 1) /* framing_flag */
> +        return AVERROR_INVALIDDATA;
> +
> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> +    st->codecpar->codec_id   = AV_CODEC_ID_VORBIS;
> +
> +    if (srate > 0) {
> +        if (st->codecpar->sample_rate &&
> +            srate != st->codecpar->sample_rate) {
> +            av_log(s, AV_LOG_ERROR, "Sample rate change is not supported\n");
> +            return AVERROR_PATCHWELCOME;
> +        }
> +
> +        st->codecpar->sample_rate = srate;
> +        avpriv_set_pts_info(st, 64, 1, srate);
> +    }
> +
> +    return 1;
> +}
> +
>  static int vorbis_header(AVFormatContext *s, int idx)
>  {
>      struct ogg *ogg = s->priv_data;
> @@ -300,6 +356,7 @@ static int vorbis_header(AVFormatContext *s, int idx)
>      struct ogg_stream *os = ogg->streams + idx;
>      struct oggvorbis_private *priv;
>      int pkt_type = os->buf[os->pstart];
> +    int ret;
>  
>      if (!os->private) {
>          os->private = av_mallocz(sizeof(struct oggvorbis_private));
> @@ -327,56 +384,18 @@ static int vorbis_header(AVFormatContext *s, int idx)
>  
>      priv->len[pkt_type >> 1]    = os->psize;
>      priv->packet[pkt_type >> 1] = av_memdup(os->buf + os->pstart, os->psize);
> +
>      if (!priv->packet[pkt_type >> 1])
>          return AVERROR(ENOMEM);
> -    if (os->buf[os->pstart] == 1) {
> -        const uint8_t *p = os->buf + os->pstart + 7; /* skip "\001vorbis" tag */
> -        unsigned blocksize, bs0, bs1;
> -        int srate;
> -        int channels;
> -
> -        if (os->psize != 30)
> -            return AVERROR_INVALIDDATA;
> -
> -        if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> -            return AVERROR_INVALIDDATA;
> -
> -        channels = bytestream_get_byte(&p);
> -        if (st->codecpar->ch_layout.nb_channels &&
> -            channels != st->codecpar->ch_layout.nb_channels) {
> -            av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> -            return AVERROR_PATCHWELCOME;
> -        }
> -        st->codecpar->ch_layout.nb_channels = channels;
> -        srate               = bytestream_get_le32(&p);
> -        p += 4; // skip maximum bitrate
> -        st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal bitrate
> -        p += 4; // skip minimum bitrate
> -
> -        blocksize = bytestream_get_byte(&p);
> -        bs0       = blocksize & 15;
> -        bs1       = blocksize >> 4;
> -
> -        if (bs0 > bs1)
> -            return AVERROR_INVALIDDATA;
> -        if (bs0 < 6 || bs1 > 13)
> -            return AVERROR_INVALIDDATA;
> -
> -        if (bytestream_get_byte(&p) != 1) /* framing_flag */
> -            return AVERROR_INVALIDDATA;
> -
> -        st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> -        st->codecpar->codec_id   = AV_CODEC_ID_VORBIS;
> -
> -        if (srate > 0) {
> -            st->codecpar->sample_rate = srate;
> -            avpriv_set_pts_info(st, 64, 1, srate);
> -        }
> -    } else if (os->buf[os->pstart] == 3) {


moving code should be in a seperate patch so any changes can be clearly
seen in the diff which does not move the code.

The output from:
libavformat/tests/seek $TICKETS/2739/lavf_ogm_seeking_borked.ogm

chanegs by:
@@ -273,7 +273,7 @@
 ret: 0         st: 2 flags:0  ts: 0.365000
 ret: 0         st: 2 flags:1 dts: 0.332000 pts: 0.332000 pos:  18959 size:   271
 ret: 0         st: 2 flags:1  ts:-0.740833
-ret: 0         st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos:   7691 size:  2519
+ret: 0         st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos:  14580 size:    39
 ret: 0         st: 3 flags:0  ts: 2.153000
 ret: 0         st: 3 flags:1 dts: 113.023000 pts: 113.023000 pos:25335211 size:    47
 ret: 0         st: 3 flags:1  ts: 1.048000
@@ -295,7 +295,7 @@
 ret: 0         st: 1 flags:1  ts: 0.200833
 ret: 0         st: 1 flags:1 dts:-0.002667 pts:-0.002667 pos:  10315 size:    32
 ret: 0         st: 2 flags:0  ts:-0.905000
-ret: 0         st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos:   7691 size:  2519
+ret: 0         st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos:  14580 size:    39
 ret: 0         st: 2 flags:1  ts: 1.989167
 ret: 0         st: 2 flags:1 dts: 1.782667 pts: 1.782667 pos: 321518 size:   241
 ret: 0         st: 3 flags:0  ts: 0.883000
Command exited with non-zero status 1

Is this correct&intended ?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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