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

[Clément Bœsch]

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);

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

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

-- 
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/20121011/00f1289b/attachment.asc>