Le jeu. 13 févr. 2025 à 15:53, Lynne <d...@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
_______________________________________________
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".
