[FFmpeg-cvslog] r17686 - trunk/libavformat/r3d.c

[Aurelien Jacobs]

Baptiste Coudurier wrote:

> Aurelien Jacobs wrote:
> > Baptiste Coudurier wrote:
> > 
> >> Hi,
> >>
> >> On 3/1/2009 7:28 AM, aurel wrote:
> >>> Author: aurel
> >>> Date: Sun Mar  1 16:28:56 2009
> >>> New Revision: 17686
> >>>
> >>> Log:
> >>> use new metadata API in r3d demuxer
> >>>
> >>> Modified:
> >>>    trunk/libavformat/r3d.c
> >>>
> >>> Modified: trunk/libavformat/r3d.c
> >>> ==============================================================================
> >>> --- trunk/libavformat/r3d.c	Sun Mar  1 15:56:27 2009	(r17685)
> >>> +++ trunk/libavformat/r3d.c	Sun Mar  1 16:28:56 2009	(r17686)
> >>> @@ -51,6 +51,7 @@ static int read_atom(AVFormatContext *s,
> >>>  static int r3d_read_red1(AVFormatContext *s)
> >>>  {
> >>>      AVStream *st = av_new_stream(s, 0);
> >>> +    char filename[258];
> >>>      int tmp, tmp2;
> >>>  
> >>>      if (!st)
> >>> @@ -92,12 +93,11 @@ static int r3d_read_red1(AVFormatContext
> >>>          av_set_pts_info(ast, 32, 1, st->time_base.den);
> >>>      }
> >>>  
> >>> -    st->filename = av_mallocz(258);
> >>> -    if (!st->filename)
> >>> -        return AVERROR(ENOMEM);
> >>> -    get_buffer(s->pb, st->filename, 257);
> >>> +    get_buffer(s->pb, filename, 257);
> >>> +    filename[sizeof(filename)-1] = 0;
> >>> +    av_metadata_set(&st->metadata, "filename", filename);
> >>>  
> >>> -    dprintf(s, "filename %s\n", st->filename);
> >>> +    dprintf(s, "filename %s\n", filename);
> >>>      dprintf(s, "resolution %dx%d\n", st->codec->width, st->codec->height);
> >>>      dprintf(s, "timescale %d\n", st->time_base.den);
> >>>      dprintf(s, "frame rate %d/%d\n",
> >> Wouldn't it be more convenient to be able to av_metadata_set using
> >> uint8_t *, avoiding memcpy/strdup ?
> > 
> > Could you be more specific ? I don't get what you mean. This is exactly
> > how av_metadata_set() works right now.
> 
> Really ? I see av_strdup in av_metadata_set.
> 
> > Or maybe you mean using av_metadata_set() with a ByteIOContext* ?
> > And if so, should it read a fixed amount of bytes (eventually adding
> > trailing 0) or should it read until it find a 0 ? In the last case,
> > it wouldn't avoid a memcpy. And in the first case, I think it would
> > anyway handle only a very limited subset of the av_metadata_set()
> > use case.
> > 
> 
> I mean:
> 
> filename = av_malloc(257);
> get_buffer(s->pb, filename, 257);
> <set trailing 0>
> av_metadata_set(&st->metadata, "filename", filename);

OK. That wasn't what I understood from your initial statement.
Now it make more sens.
But it don't fit as well in all other use case. Many demuxers
would have to do the av_malloc()/memcpy() in addition to the
code they already have. So in practice this will duplicate
the strdup in many demuxers instead of doing it in a central
place. This also means that the user app will have to do the
malloc itself, and only using ffmpeg's allocation functions,
but never free itself. This could be an important source of
confusion and bugs.
So, even if it would simplify a few demuxers, all in all, I doubt
this would be a clear gain.

Aurel