Le ven. 30 mai 2025 à 19:44, Andreas Rheinhardt
<andreas.rheinha...@outlook.com> a écrit :
>
> Romain Beauxis:
> > ---
> > libavcodec/vorbis_parser.h | 11 ++++
> > libavcodec/vorbisdec.c | 75 +++++++++++++---------
> > libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
> > tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> > tests/ref/fate/trac-2739.txt | 4 +-
> > 5 files changed, 121 insertions(+), 39 deletions(-)
> >
> > diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> > index 789932ac49..b176fe536c 100644
> > --- a/libavcodec/vorbis_parser.h
> > +++ b/libavcodec/vorbis_parser.h
> > @@ -30,6 +30,17 @@
> >
> > 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 adbd726183..a4b159ba9b 100644
> > --- a/libavcodec/vorbisdec.c
> > +++ b/libavcodec/vorbisdec.c
> > @@ -43,6 +43,7 @@
> > #include "vorbis.h"
> > #include "vorbisdsp.h"
> > #include "vorbis_data.h"
> > +#include "vorbis_parser.h"
> > #include "xiph.h"
> >
> > #define V_NB_BITS 8
> > @@ -1778,47 +1779,59 @@ 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);
> >
> > - if (*buf == 1 && buf_size > 7) {
> > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > - return ret;
> > + new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
> > + avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
> >
> > - 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;
> > - }
> > + if (new_extradata) {
> > + header = new_extradata->header;
> > + setup = new_extradata->setup;
> >
> > - 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->header_size > 7 && *header == 1) {
> > + if ((ret = init_get_bits8(
> > + gb, header + 1,
> > + new_extradata->header_size - 1)) < 0)
> > + return ret;
> >
> > - avctx->sample_rate = vc->audio_samplerate;
> > - return buf_size;
> > - }
> > + 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;
> > + }
> >
> > - if (*buf == 3 && buf_size > 7) {
> > - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> > - return buf_size;
> > - }
> > + 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 (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > - return ret;
> > + avctx->sample_rate = vc->audio_samplerate;
> > + }
> >
> > - if ((ret = vorbis_parse_setup_hdr(vc))) {
> > - av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> > - vorbis_free(vc);
> > - return ret;
> > + 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;
> > +
> > + 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 62cc2da6de..f8e66e8127 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -255,12 +255,19 @@ 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)
> > @@ -433,7 +440,10 @@ 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;
> > @@ -496,10 +506,61 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> > if (duration < 0) {
> > os->pflags |= AV_PKT_FLAG_CORRUPT;
> > return 0;
> > - } else if (flags & VORBIS_FLAG_COMMENT) {
> > - vorbis_update_metadata(s, idx);
> > + }
> > +
> > + 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;
> > +
> > 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;
> > }
> >
> > @@ -521,7 +582,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> > priv->final_duration += os->pduration;
> > }
> >
> > - return 0;
> > + return skip_packet;
> > }
> >
> > const struct ogg_codec ff_vorbis_codec = {
>
> There are multiple issues with this patch:
Thank you for your feedback.
> 1. The side data structures are not padded, leading to
> heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
Do you have a pointer to this issue? Is there a failing test here:
https://fate.ffmpeg.org/ ?
> 2. The side data structures are not flat and therefore not suitable for
> use as AVPacketSideData. (The setup and header arrays are currently
> owned by the demuxer, yet an AVPacket is supposed to be valid on its
> own. But this side data becomes invalid when the demuxer encounters a
> new side data (and reallocates its internal buffers) or when the demuxer
> is closed.)
I can work on that.
> 3. The vorbis_new_extradata name does not abide by our naming
> conventions; given that this is a public header, it would need an AV prefix.
Noted.
> 4. You forgot to add an APIchanges entry and bump lavc minor for your
> addition.
Will add
> 5. You forgot to add stddef.h to vorbis_parser.h.
Will add
> 6. You should be aware that our parsing API does not work with
> AVPackets, but with raw buffers. It (or rather: parse_packet() in
> libavformat/demux.c) therefore has a tendency to put the side data to
> the wrong packets or completely omit it (this happens when the parser
> introduces a delay). Vorbis-in-ogg does not enable the parser, so it is
> (currently) unaffected by this.
Noted, glad it's not impacted. I'll have a look at the corresponding code.
I'll send a fix for these as part of the next row of patches. I'll
make sure to CC/ you so I can get your opinion before it gets merged.
-- Romain
> - Andreas
>
> _______________________________________________
> 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".
_______________________________________________
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".
