[FFmpeg-devel] [PATCH 2/4] lavd: make lavfi device export the metadata up to the AVFrame.
Clément Bœsch
ubitux at gmail.com
Sun Oct 14 14:34:34 CEST 2012
On Sun, Oct 14, 2012 at 12:12:38PM +0200, Nicolas George wrote:
> Le duodi 22 vendémiaire, an CCXXI, Clément Bœsch a écrit :
> > On Thu, Oct 11, 2012 at 11:22:53PM +0200, Clément Bœsch wrote:
> > > On Wed, Oct 10, 2012 at 12:55:11AM +0200, Clément Bœsch wrote:
> > > > TODO: lavc minor bump?
> > > > ---
> > > > libavcodec/avcodec.h | 1 +
> > > > libavcodec/utils.c | 7 +++++++
> > > > libavdevice/lavfi.c | 9 +++++++++
> > > > 3 files changed, 17 insertions(+)
> > > >
> > >
> > > After a discussion on IRC, we are considering something more complex but
> > > more solid:
> > > - in lavd/lavfi: convert each buffer ref metadata key/value into side
> > > data type "meta" like let's say "key\0value\0"
> > > - put some metadata dict in the raw a/v contexts (just like the tiff
> > > decoder), and construct it based on the "meta" side data of each
> > > packets. (AVCodecInternal was also mentioned, I need to look how it
> > > helps)
> > >
> >
> > Here we go, here is the V2. Should be better than the previous hack.
> >
> > --
> > Clément B.
>
> > From 5924da49cf4158f7ee4cdbfea22b822137b05be3 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
> > Date: Tue, 9 Oct 2012 23:01:07 +0200
> > Subject: [PATCH 2/4] lavd: make lavfi device export the metadata up to the
> > AVFrame.
> >
> > TODO: lavc minor bump + APIChanges
> > ---
> > libavcodec/avcodec.h | 6 ++++++
> > libavcodec/internal.h | 7 +++++++
> > libavcodec/pcm.c | 8 ++++++--
> > libavcodec/rawdec.c | 6 ++++++
> > libavcodec/utils.c | 22 ++++++++++++++++++++++
> > libavdevice/lavfi.c | 20 ++++++++++++++++++++
> > 6 files changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 3c3dad3..9370a56 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -941,6 +941,12 @@ enum AVPacketSideDataType {
> > * @endcode
> > */
> > AV_PKT_DATA_JP_DUALMONO,
> > +
> > + /**
> > + * A list of zero terminated key/value strings. There is no end marker for
> > + * the list, so it is required to rely on the side data size to stop.
> > + */
> > + AV_PKT_DATA_METATAGS,
>
> We may need to add a gap in the values, to avoid ABI compatibility issues
> with the fork. Unfortunately, the problem is already there with
> AV_PKT_DATA_JP_DUALMONO. We may want to change that one very soon, while it
> is still less than two months old and can not be considered a severe ABI
> break.
>
I think it's ok, a bit earlier in the enum you have
AV_PKT_DATA_SKIP_SAMPLES=70
[...]
> > diff --git a/libavcodec/rawdec.c b/libavcodec/rawdec.c
> > index d426f43..2275029 100644
> > --- a/libavcodec/rawdec.c
> > +++ b/libavcodec/rawdec.c
> > @@ -25,6 +25,7 @@
> > */
> >
> > #include "avcodec.h"
> > +#include "internal.h"
> > #include "raw.h"
> > #include "libavutil/avassert.h"
> > #include "libavutil/common.h"
> > @@ -258,6 +259,9 @@ static int raw_decode(AVCodecContext *avctx,
> > }
> > }
> >
> > + ff_set_metadata_from_side_data(avctx->coded_frame, avpkt);
> > + frame->metadata = avctx->coded_frame->metadata;
> > +
>
> Are we to assume that the side-data metadata will only be taken into account
> if it comes with PCM and rawvideo? That is not completely satisfactory, but
> it is enough for the needs at hand, and it solves the risk of memory leak.
>
Yes that's a drawback with this solution. Right now, it's only necessary
for the lavfi device anyway, I don't know yet if we will need it
elsewhere. If you have another idea in mind to have it in all decoders at
once...
> > *data_size = sizeof(AVPicture);
> > return buf_size;
> > }
> > @@ -266,6 +270,8 @@ static av_cold int raw_close_decoder(AVCodecContext *avctx)
> > {
> > RawVideoContext *context = avctx->priv_data;
> >
> > + if (avctx->coded_frame)
> > + av_dict_free(&avctx->coded_frame->metadata);
> > av_freep(&context->buffer);
> > return 0;
> > }
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 183776a..d987e45 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -2590,3 +2590,25 @@ int avcodec_is_open(AVCodecContext *s)
> > {
> > return !!s->internal;
> > }
> > +
> > +int ff_set_metadata_from_side_data(AVFrame *frame, AVPacket *pkt)
> > +{
> > + int size;
> > + const uint8_t *side_meta;
> > + const uint8_t *end;
> > +
> > + av_dict_free(&frame->metadata);
> > + side_meta = av_packet_get_side_data(pkt, AV_PKT_DATA_METATAGS, &size);
> > + if (!side_meta)
> > + return 0;
> > + end = side_meta + size;
> > + while (side_meta < end) {
> > + const uint8_t *key = side_meta;
> > + const uint8_t *val = side_meta + strlen(key) + 1;
> > + int ret = av_dict_set(&frame->metadata, key, val, 0);
> > + if (ret < 0)
> > + return ret;
> > + side_meta = val + strlen(val) + 1;
> > + }
> > + return 0;
> > +}
> > diff --git a/libavdevice/lavfi.c b/libavdevice/lavfi.c
> > index 944794f..faf918a 100644
> > --- a/libavdevice/lavfi.c
> > +++ b/libavdevice/lavfi.c
> > @@ -27,6 +27,7 @@
> >
> > #include "float.h" /* DBL_MIN, DBL_MAX */
> >
> > +#include "libavutil/bprint.h"
> > #include "libavutil/log.h"
> > #include "libavutil/mem.h"
> > #include "libavutil/opt.h"
> > @@ -339,6 +340,25 @@ static int lavfi_read_packet(AVFormatContext *avctx, AVPacket *pkt)
> > memcpy(pkt->data, ref->data[0], size);
> > }
> >
> > + if (ref->metadata) {
> > + uint8_t *meta;
> > + AVDictionaryEntry *e = NULL;
> > + AVBPrint meta_buf;
> > +
> > + av_bprint_init(&meta_buf, 0, AV_BPRINT_SIZE_UNLIMITED);
> > + while ((e = av_dict_get(ref->metadata, "", e, AV_DICT_IGNORE_SUFFIX))) {
> > + av_bprintf(&meta_buf, "%s", e->key);
> > + av_bprint_chars(&meta_buf, '\0', 1);
> > + av_bprintf(&meta_buf, "%s", e->value);
> > + av_bprint_chars(&meta_buf, '\0', 1);
> > + }
> > + meta = av_packet_new_side_data(pkt, AV_PKT_DATA_METATAGS, meta_buf.len);
>
> You forgot to check that the bprint buffer was not truncated.
>
Mmh I never used av_bprint_is_complete()... 'will fix soon.
> > + if (!meta)
> > + return AVERROR(ENOMEM);
>
> You are leaking the bprint buffer if this error happens.
>
I'll send a new patch in a while, thanks
[...]
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121014/73556e87/attachment.asc>
More information about the ffmpeg-devel
mailing list