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

[Clément Bœsch]

On Sun, Oct 14, 2012 at 12:40:25PM +0200, Stefano Sabatini wrote:
> 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.
> 

Sure, the question is to know if we aren't doing that stuff above. I'm
going to investigate a little a send a new patch.

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

Sure OK. Changed locally

[...]

-- 
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/20121016/c8398d60/attachment.asc>