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

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

Le mar. 13 mai 2025 à 14:23, Michael Niedermayer <michael@niedermayer.cc> a
écrit :
>
> 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 ?

Sorry I thought that this sample was part of FATE but it isn't their nor is
it linked in the corresponding ticket.

It would be nice if there was a definite set of samples/tests to check
before sending patches, I do my best to check my work against the
established set of validation checks but I am unable to test non-existing
files!

Would you provide the file for reference? Is it possible to have an
extensive set of the checks that you are intending to see passed before
considering for inclusion?

I have the split up patchset ready to re-submit when I can confirm that it
works as expected.

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