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

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

Le sam. 17 mai 2025 à 17:07, Michael Niedermayer
<michael@niedermayer.cc> a écrit :
>
> On Sat, May 17, 2025 at 01:10:26PM -0500, Romain Beauxis wrote:
> > 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.
>
> samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2739/lavf_ogm_seeking_borked.ogm

Thanks. I just had a look. It's hard to know what's really going on
because this stream is highly invalid:
[ogg @ 0x12d804080] Headers mismatch for stream 1: expected 3 received 2.
[ogg @ 0x12d804080] Headers mismatch for stream 2: expected 3 received 2.

The logic for packet passing is changed with this patch series and new
headers from subsequent chained streams are only passed with the first
data packet following the headers.

I haven't debugged down to the de-packetization but, a typical change
would be: if the stream is invalid and has a series of headers without
any data packet in-between, then the decoder would never see the
intermediate headers with the new changes.

However, I have rewritten my changes to actually pass both header and
setup packet as extradata. This way, the decoder will see whatever we
are able to pass as header and setup packet from the parser.

This should make sure that we are sticking as close as possible to the
existing behavior.

The seek diff is still the same with it but that makes me more
confident that this is, indeed, intended.

-- Romain

> >
> > It would be nice if there was a definite set of samples/tests to check
> > before sending patches,
>
> yes, ideally every (or most) fixed tickets would result in a new fate test
>
> It is something that someone should do as a STF task or as a volunteer.
> Its not something i have teh time or patience for
>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Republics decline into democracies and democracies degenerate into
> despotisms. -- Aristotle
> _______________________________________________
> 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".
_______________________________________________
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".