[FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
Michael Niedermayer
michael at niedermayer.cc
Tue May 13 22:23:39 EEST 2025
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250513/2ef17da4/attachment.sig>
More information about the ffmpeg-devel
mailing list