[FFmpeg-devel] [PATCH v4 3/6] Pass secondary ogg/opus chained streams metadata.

[Romain Beauxis]

Le jeu. 13 févr. 2025 à 15:53, Lynne <dev at lynne.ee> a écrit :
>
> On 10/02/2025 20:25, Romain Beauxis wrote:
> > These changes parse ogg/opus comment in secondary chained ogg/opus
> > streams and attach them as extradata to the corresponding packet.
> >
> > They can then be decoded in the opus decoder and attached to the next
> > decoded frame.
> >
> > libavformat/oggparseopus.c: Parse comments from
> >   secondary chained ogg/opus streams and pass them as ogg stream new_metadata.
> >
> > libavcodec/opus/dec.c: Unpack comments from secondary chained ogg/opus
> > streams and attach them to the next decoded frame.
> > ---
> >   libavcodec/opus/dec.c      | 25 ++++++++++++++++++++++---
> >   libavformat/oggparseopus.c | 16 +++++++++++++++-
> >   2 files changed, 37 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavcodec/opus/dec.c b/libavcodec/opus/dec.c
> > index 88a650c81c..cddcefcb5f 100644
> > --- a/libavcodec/opus/dec.c
> > +++ b/libavcodec/opus/dec.c
> > @@ -125,6 +125,8 @@ typedef struct OpusContext {
> >       AVFloatDSPContext *fdsp;
> >       float   gain;
> >
> > +    AVDictionary *pending_metadata;
> > +
> >       OpusParseContext p;
> >   } OpusContext;
> >
> > @@ -485,12 +487,24 @@ static int opus_decode_packet(AVCodecContext *avctx, AVFrame *frame,
> >       int decoded_samples = INT_MAX;
> >       int delayed_samples = 0;
> >       int i, ret;
> > +    size_t size;
> > +    const uint8_t *side_metadata;
> >
> > -    if (buf_size > 8 && (
> > -       !memcmp(buf, "OpusHead", 8) ||
> > -       !memcmp(buf, "OpusTags", 8)))
> > +    if (buf_size > 8 && !memcmp(buf, "OpusHead", 8))
> >           return buf_size;
> >
> > +    if (buf_size > 8 && !memcmp(buf, "OpusTags", 8)) {
> > +        /* New metadata */
> > +        side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size);
> > +        if (side_metadata) {
> > +            av_dict_free(&c->pending_metadata);
> > +            ret = av_packet_unpack_dictionary(side_metadata, size, &c->pending_metadata);
> > +            if (ret < 0)
> > +                return ret;
> > +        }
> > +        return buf_size;
> > +    }
>
> This is definitely not the right way to go about it. You're changing the
> packet layout too, which can potentially break existing users.

Thanks for looking into this!

> What you should do instead is to either create a linearized dictionary
> (e.g. <keylength><key><value_len><value> in a buffer), and pass that as
> extradata, or add a dictionary field to AVPacket, which would then be
> copied over to the frame after decoding, similar to how PTS, duration,
> etc. are carried over from packets.

I'm confused: isn't it exactly what the ogg demuxer is already doing:

    if (os->new_metadata) {
        ret = av_packet_add_side_data(pkt, AV_PKT_DATA_METADATA_UPDATE,
                                      os->new_metadata, os->new_metadata_size);
        if (ret < 0)
            return ret;

        os->new_metadata      = NULL;
        os->new_metadata_size = 0;
    }

(Note that this code is already in the ogg decoder and not part of
those changes)

This is also in response (and personal agreement) with Marvin's
feedback here: https://ffmpeg.org/pipermail/ffmpeg-devel/2025-January/338993.html

> Having to add custom handling to each codec to handle metadata updates
> is not good design.

Could you explain more in detail what you mean?

I really am confused, the mechanism here is generic, using a flat
dictionary stored as AV_PKT_DATA_METADATA_UPDATE extra data and copied
over to the frame after decoding.

This seems like what you describe? What am I missing?

Thanks,
-- Romain