[FFmpeg-devel] [PATCH 1/4] lavfi: add metadata to buffer ref.

[Stefano Sabatini]

On date Thursday 2012-10-11 22:05:11 +0200, Clément Bœsch encoded:
> On Thu, Oct 11, 2012 at 11:34:24AM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2012-10-10 00:55:10 +0200, Clément Bœsch encoded:
> > > From: Thomas Kühnel <kuehnelth at googlemail.com>
> > > 
> > > Signed-off-by: Thomas Kühnel <kuehnelth at googlemail.com>
> > > Signed-off-by: Clément Bœsch <ubitux at gmail.com>
> > > ---
> > >  libavfilter/avcodec.c  | 6 ++++++
> > >  libavfilter/avfilter.h | 2 ++
> > >  libavfilter/buffer.c   | 8 ++++++++
> > >  3 files changed, 16 insertions(+)
> > > 
> > > diff --git a/libavfilter/avcodec.c b/libavfilter/avcodec.c
> > > index a0f8b69..411f1dd 100644
> > > --- a/libavfilter/avcodec.c
> > > +++ b/libavfilter/avcodec.c
> > > @@ -33,6 +33,9 @@ int avfilter_copy_frame_props(AVFilterBufferRef *dst, const AVFrame *src)
> > >      dst->pos    = av_frame_get_pkt_pos(src);
> > >      dst->format = src->format;
> > >  
> > 
> > > +    dst->metadata = NULL;
> > > +    av_dict_copy(&dst->metadata, src->metadata, 0);
> > > +
> > 
> > leak if dst->metadata is already defined?
> > 
> 
> I was not sure about that one: don't we have cases were the destination
> buffer ref could be uninitialized? Like let's say this scenario:
> 
>     AVFilterBufferRef refbak;
> 
>     avfilter_copy_buf_props(&refbak, ref);
>     [...]
>     avfilter_copy_buf_props(ref, &refbak);

I think it is safer to 0-init the data, and avoid a possible leak.

> 
> > >      switch (dst->type) {
> > >      case AVMEDIA_TYPE_VIDEO:
> > >          dst->video->w                   = src->width;
> > > @@ -121,6 +124,9 @@ int avfilter_copy_buf_props(AVFrame *dst, const AVFilterBufferRef *src)
> > >      dst->format  = src->format;
> > >      av_frame_set_pkt_pos(dst, src->pos);
> > >  
> > > +    dst->metadata = NULL;
> > > +    av_dict_copy(&dst->metadata, src->metadata, 0);
> > > +
> > >      switch (src->type) {
> > >      case AVMEDIA_TYPE_VIDEO:
> > >          av_assert0(src->video);
> > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > index 510f28a..c171826 100644
> > > --- a/libavfilter/avfilter.h
> > > +++ b/libavfilter/avfilter.h
> > > @@ -180,6 +180,8 @@ typedef struct AVFilterBufferRef {
> > >      int perms;                  ///< permissions, see the AV_PERM_* flags
> > >  
> > >      enum AVMediaType type;      ///< media type of buffer data
> > > +
> > > +    AVDictionary *metadata;
> > 
> > Missing doxy, explaining what it this all about (a line should be enough).
> > 
> 

> Added locally:
>     ///< metadata for communication between filters and API user

I'd keep it more generic, since communication may happens between
filters, thus unobservable to the API user:
///< dictionary containing metadata key=value tags

> 
> > >  } AVFilterBufferRef;
> > >  
> > >  /**
> > > diff --git a/libavfilter/buffer.c b/libavfilter/buffer.c
> > > index fc65b82..ae1867f 100644
> > > --- a/libavfilter/buffer.c
> > > +++ b/libavfilter/buffer.c
> > > @@ -54,6 +54,10 @@ AVFilterBufferRef *avfilter_ref_buffer(AVFilterBufferRef *ref, int pmask)
> > >      if (!ret)
> > >          return NULL;
> > >      *ret = *ref;
> > > +
> > > +    ret->metadata = NULL;
> > > +    av_dict_copy(&ret->metadata, ref->metadata, 0);
> > > +
> > >      if (ref->type == AVMEDIA_TYPE_VIDEO) {
> > >          ret->video = av_malloc(sizeof(AVFilterBufferRefVideoProps));
> > >          if (!ret->video) {
> > > @@ -172,6 +176,7 @@ void avfilter_unref_buffer(AVFilterBufferRef *ref)
> > >          av_freep(&ref->video->qp_table);
> > >      av_freep(&ref->video);
> > >      av_freep(&ref->audio);
> > > +    av_dict_free(&ref->metadata);
> > >      av_free(ref);
> > >  }
> > >  
> > > @@ -197,6 +202,9 @@ void avfilter_copy_buffer_ref_props(AVFilterBufferRef *dst, AVFilterBufferRef *s
> > >      case AVMEDIA_TYPE_AUDIO: *dst->audio = *src->audio; break;
> > >      default: break;
> > >      }
> > > +
> > > +    av_dict_free(&dst->metadata);
> > > +    av_dict_copy(&dst->metadata, src->metadata, 0);
> > >  }
> > >  
> > >  AVFilterBufferRef *ff_copy_buffer_ref(AVFilterLink *outlink,
> > 
> > TODO: APIChanges entry, lavfi minor bump.
> > 
> 
> TODO added to the description commit so I don't forget.
> 
> > LGTM otherwise, thanks.
> 
> Will push when the first issue is sorted out, thanks for the review.

LGTM otherwise.
-- 
FFmpeg = Fast and Fascinating Minimal Programmable Erudite Ghost