On Sat, May 17, 2025 at 01:10:26PM -0500, Romain Beauxis wrote:
> Le mar. 13 mai 2025 à 14:23, Michael Niedermayer <mich...@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 > > 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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
