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

[Lynne]

On 14/02/2025 00:27, Romain Beauxis wrote:
> 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;
>      }

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.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?

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xA2FEA5F03F034464.asc
Type: application/pgp-keys
Size: 624 bytes
Desc: OpenPGP public key
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250214/ba4473b7/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250214/ba4473b7/attachment.sig>