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

[Baptiste Coudurier]

Aurelien Jacobs wrote:
> 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.

Humm, yes I agree, though I find the need to declare a local buffer
ugly, and when we remove the 1024 limit for metadata, this will mean
malloc and free. We need a good solution ...

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org