Hi all,
Le mer. 4 juin 2025 à 11:58, Romain Beauxis <romain.beau...@gmail.com> a écrit :
>
> This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
> memory storage for the extradata.
>
> PR review comments addressed:
> * Use flat memory bytestream
> * Re-use existing xiph extradata layout
Is there any interest in reviewing this patch?
It's holding the second series that fixes chained ogg stream metadata parsing.
Better support for chained ogg streams would really make life easier
for a bunch of ffmpeg and ffmpeg libraries users.
Thanks,
-- Romain
> ---
> libavcodec/vorbisdec.c | 42 ++++++++---
> libavformat/oggparsevorbis.c | 83 +++++++++++++++++++++-
> tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> 3 files changed, 114 insertions(+), 14 deletions(-)
>
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index adbd726183..84879462a1 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -1778,11 +1778,40 @@ 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;
> + const uint8_t *new_extradata;
> + const uint8_t *header_start[3];
> + int header_len[3] = {0, 0, 0};
> + const uint8_t *header;
> + int header_size = 0;
> + const uint8_t *comment;
> + int comment_size = 0;
> + const uint8_t *setup;
> + int setup_size = 0;
>
> 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)
> + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> + &new_extradata_size);
> +
> + if (new_extradata) {
> + ret = avpriv_split_xiph_headers(new_extradata, new_extradata_size,
> + 30, header_start, header_len);
> + if (ret < 0)
> + return ret;
> +
> + header = header_start[0];
> + header_size = header_len[0];
> +
> + comment = header_start[1];
> + comment_size = header_len[1];
> +
> + setup = header_start[2];
> + setup_size = header_len[2];
> + }
> +
> + if (header_size > 7 && *header == 1) {
> + if ((ret = init_get_bits8(gb, header + 1, header_size - 1)) < 0)
> return ret;
>
> vorbis_free(vc);
> @@ -1801,16 +1830,14 @@ static int vorbis_decode_frame(AVCodecContext *avctx,
> AVFrame *frame,
> }
>
> avctx->sample_rate = vc->audio_samplerate;
> - return buf_size;
> }
>
> - if (*buf == 3 && buf_size > 7) {
> + if (comment_size > 7 && *comment == 3) {
> av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> - return buf_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 (setup_size > 7 && *setup == 5 && vc->channel_residues && !vc->modes)
> {
> + if ((ret = init_get_bits8(gb, setup + 1, setup_size - 1)) < 0)
> return ret;
>
> if ((ret = vorbis_parse_setup_hdr(vc))) {
> @@ -1818,7 +1845,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 62cc2da6de..7859ec5a51 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -215,6 +215,12 @@ struct oggvorbis_private {
> AVVorbisParseContext *vp;
> int64_t final_pts;
> int final_duration;
> + uint8_t *header;
> + int header_size;
> + uint8_t *comment;
> + int comment_size;
> + uint8_t *setup;
> + int setup_size;
> };
>
> static int fixup_vorbis_headers(AVFormatContext *as,
> @@ -260,6 +266,10 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
> av_vorbis_parse_free(&priv->vp);
> for (i = 0; i < 3; i++)
> av_freep(&priv->packet[i]);
> +
> + av_freep(&priv->header);
> + av_freep(&priv->comment);
> + av_freep(&priv->setup);
> }
> }
>
> @@ -434,6 +444,9 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> struct ogg_stream *os = ogg->streams + idx;
> struct oggvorbis_private *priv = os->private;
> int duration, flags = 0;
> + int skip_packet = 0;
> + int ret, new_extradata_size;
> + PutByteContext pb;
>
> if (!priv->vp)
> return AVERROR_INVALIDDATA;
> @@ -496,10 +509,50 @@ 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;
> +
> + ret = av_reallocp(&priv->header, os->psize);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(priv->header, os->buf + os->pstart, os->psize);
> + priv->header_size = os->psize;
> +
> + skip_packet = 1;
> + }
> +
> + if (flags & VORBIS_FLAG_COMMENT) {
> + ret = vorbis_update_metadata(s, idx);
> + if (ret < 0)
> + return ret;
> +
> + ret = av_reallocp(&priv->comment, os->psize);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(priv->comment, os->buf + os->pstart, os->psize);
> + priv->comment_size = os->psize;
> +
> flags = 0;
> + skip_packet = 1;
> + }
> +
> + if (flags & VORBIS_FLAG_SETUP) {
> + ret = av_reallocp(&priv->setup, os->psize);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(priv->setup, os->buf + os->pstart, os->psize);
> + priv->setup_size = os->psize;
> +
> + skip_packet = 1;
> }
> +
> os->pduration = duration;
> }
>
> @@ -521,7 +574,31 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> priv->final_duration += os->pduration;
> }
>
> - return 0;
> + if (priv->header && priv->comment && priv->setup) {
> + new_extradata_size = priv->header_size + priv->comment_size +
> priv->setup_size + 6;
> +
> + ret = av_reallocp(&os->new_extradata, new_extradata_size);
> + if (ret < 0)
> + return ret;
> +
> + os->new_extradata_size = new_extradata_size;
> + bytestream2_init_writer(&pb, os->new_extradata, new_extradata_size);
> + bytestream2_put_be16(&pb, priv->header_size);
> + bytestream2_put_buffer(&pb, priv->header, priv->header_size);
> + bytestream2_put_be16(&pb, priv->comment_size);
> + bytestream2_put_buffer(&pb, priv->comment, priv->comment_size);
> + bytestream2_put_be16(&pb, priv->setup_size);
> + bytestream2_put_buffer(&pb, priv->setup, priv->setup_size);
> +
> + av_freep(&priv->header);
> + priv->header_size = 0;
> + av_freep(&priv->comment);
> + priv->comment_size = 0;
> + av_freep(&priv->setup);
> + priv->comment_size = 0;
> + }
> +
> + return skip_packet;
> }
>
> const struct ogg_codec ff_vorbis_codec = {
> diff --git a/tests/ref/fate/ogg-vorbis-chained-meta.txt
> b/tests/ref/fate/ogg-vorbis-chained-meta.txt
> index b7a97c90e2..1206f86c1f 100644
> --- a/tests/ref/fate/ogg-vorbis-chained-meta.txt
> +++ b/tests/ref/fate/ogg-vorbis-chained-meta.txt
> @@ -6,10 +6,7 @@ Stream ID: 0, frame PTS: 128, metadata: N/A
> Stream ID: 0, packet PTS: 704, packet DTS: 704
> Stream ID: 0, frame PTS: 704, metadata: N/A
> Stream ID: 0, packet PTS: 0, packet DTS: 0
> -Stream ID: 0, packet PTS: 0, packet DTS: 0
> Stream ID: 0, new metadata: encoder=Lavc61.19.100 libvorbis:title=Second
> Stream
> -Stream ID: 0, packet PTS: 0, packet DTS: 0
> -Stream ID: 0, packet PTS: 0, packet DTS: 0
> Stream ID: 0, frame PTS: 0, metadata: N/A
> Stream ID: 0, packet PTS: 128, packet DTS: 128
> Stream ID: 0, frame PTS: 128, metadata: N/A
> --
> 2.39.5 (Apple Git-154)
>
_______________________________________________
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".
