Re: [FFmpeg-devel] [PATCH v5 1/6] libavcodec: Add generic metadata injection using AV_PKT_DATA_METADATA_UPDATE

[Romain Beauxis <romain.beauxis@gmail.com>]

Le mar. 18 févr. 2025 à 17:28, Lynne <dev@lynne.ee> a écrit :
>
> On 17/02/2025 17:19, Romain Beauxis wrote:
> > libavcodec/decode.c: intercept `AV_PKT_DATA_METADATA_UPDATE` packet extra data,
> > attach them to the next decoded frame.
> >
> > The metadata needs to be cached because there is no guarantee that each packet
> > will generate a decoded frame immediately.
> >
> > `AV_PKT_DATA_METADATA_UPDATE` does not seem used in `libavcodec` at the moment
> > so regression risks seem low.
> >
> > This generic mechanism could be reused to support more in-band metadata update
> > in the future.
> >
> > ---
> >   libavcodec/decode.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index cac7e620d2..96e2f0ce95 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -97,6 +97,8 @@ typedef struct DecodeContext {
> >       int lcevc_frame;
> >       int width;
> >       int height;
> > +
> > +    AVDictionary *pending_metadata;
> >   } DecodeContext;
> >
> >   static DecodeContext *decode_ctx(AVCodecInternal *avci)
> > @@ -729,6 +731,8 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >   {
> >       AVCodecInternal *avci = avctx->internal;
> >       DecodeContext     *dc = decode_ctx(avci);
> > +    const uint8_t *side_metadata;
> > +    size_t size;
> >       int ret;
> >
> >       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > @@ -746,6 +750,14 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >           ret = av_packet_ref(avci->buffer_pkt, avpkt);
> >           if (ret < 0)
> >               return ret;
> > +
> > +        side_metadata = av_packet_get_side_data(avpkt, AV_PKT_DATA_METADATA_UPDATE, &size);
> > +        if (side_metadata) {
> > +            av_dict_free(&dc->pending_metadata);
> > +            ret = av_packet_unpack_dictionary(side_metadata, size, &dc->pending_metadata);
> > +            if (ret < 0)
> > +                return ret;
> > +        }
> >       } else
> >           dc->draining_started = 1;
> >
> > @@ -815,6 +827,7 @@ fail:
> >   int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> >   {
> >       AVCodecInternal *avci = avctx->internal;
> > +    DecodeContext     *dc = decode_ctx(avci);
> >       int ret;
> >
> >       if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > @@ -887,6 +900,12 @@ int ff_decode_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> >           }
> >       }
> >   #endif
> > +
> > +    if (dc->pending_metadata) {
> > +        av_dict_copy(&frame->metadata, dc->pending_metadata, AV_DICT_APPEND);
> > +        av_dict_free(&dc->pending_metadata);
> > +    }
> > +
> >       return 0;
> >   fail:
> >       av_frame_unref(frame);
> > @@ -2314,4 +2333,5 @@ void ff_decode_internal_uninit(AVCodecContext *avctx)
> >       DecodeContext *dc = decode_ctx(avci);
> >
> >       av_refstruct_unref(&dc->lcevc);
> > +    av_dict_free(&dc->pending_metadata);
> >   }
>
> btw, jamrial mentioned that AVSTREAM_EVENT_FLAG_METADATA_UPDATED exists.
> I only see it used to inform API users that metadata has been updated,
> but you should set the flag.
>
> Other than that, I'm fine with the patchset. You should resend it with
> the change, and if there are no comments, I'll merge it in a few days.

Thank you for the review.

Thanks for pointing that out. Since ogg vorbis already supports
AVSTREAM_EVENT_FLAG_METADATA_UPDATED I think it would make sense to
favor our the whole process from there.

However, I have some questions about how this is implemented in vorbis.

The vorbis implementation:
* Parses new comment metadata and adds them to the AVStream's metadata
* Entries are appended in this case there is already an existing one:

         if (av_dict_get(*m, t, NULL, 0))
            av_dict_set(m, t, ";", AV_DICT_APPEND);
        av_dict_set(m, t, v, AV_DICT_APPEND);

* Then AVSTREAM_EVENT_FLAG_METADATA_UPDATED is set on the AVStream.

I can see a couple of issues:
* This is pretty unexpected behavior. Any consumer of the stream's
metadata would expect a single title, not an accumulation of all
previously parsed titles
* This is also a memory leak.

Should we make it so the stream's duplicated metadata are taking the
last value instead?

If so, I'm happy to include this change in the patchset and submit an
updated one which uses the same factored out mechanism in vorbis, flac
and opus.

-- 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-request@ffmpeg.org with subject "unsubscribe".