On 14/02/2025 00:27, Romain Beauxis wrote:
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; }
OpusHead et. al. shouldn't appear in the data of AVPacket ever.
(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.htmlHaving 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?
You have code in each decoder to handle this. You shouldn't - the generic decode.c framework should forward the updates. Decoders shouldn't care about what a demuxer did, they simply decode.
OpenPGP_0xA2FEA5F03F034464.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
_______________________________________________ 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".
