[FFmpeg-devel] [PATCH] Store Major brand, Minor version and compatible brands of a mov file using the metadata API

[Baptiste Coudurier]

Hi,

On 09/09/2009 12:03 AM, haim alon wrote:
> Hi,
>
> On Tue, Sep 8, 2009 at 8:34 PM, Baptiste Coudurier<
> baptiste.coudurier at gmail.com>  wrote:
>
>> On 09/08/2009 04:55 AM, haim alon wrote:
>>
>>> Hi,
>>>
>>> On Tue, Sep 8, 2009 at 11:54 AM, Diego Biurrun<diego at biurrun.de>   wrote:
>>>
>>>   On Mon, Sep 07, 2009 at 11:55:25AM +0300, haim alon wrote:
>>>>> --- libavformat/mov.c (revision 19787)
>>>>> +++ libavformat/mov.c (working copy)
>>>>> @@ -485,15 +485,41 @@
>>>>>
>>>>> +    for (i=0; i<   numCompBrand; i++) { /*compatible brands*/
>>>>>
>>>> nit: i = 0
>>>>
>>>>
>>>>   Done.
>>> Following Reimar's remark,  I also added a special check for the
>>> compatible
>>> name string - if there is a NULL character, this brand is skipped and a
>>> warning message is logged.
>>>
>>> Attached an updated patch.
>>>
>> Please make the patch coding style and variable naming scheme matching the
>> one used in the same file.
>>
>> compat_brand, minor_ver, etc...
>>
>>
> Done

What about variable names ?

>>> [...]
>>>
>>>       uint32_t type = get_le32(pb);
>>>
>>>       if (type != MKTAG('q','t',' ',' '))
>>>           c->isom = 1;
>>> -    av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand:
>>> %.4s\n",(char *)&type);
>>> -    get_be32(pb); /* minor version */
>>> -    url_fskip(pb, atom.size - 8);
>>> +    av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand: %.4s\n",
>>> (char *)&type);
>>> +    av_strlcpy(majorBrandStr, (char*)&type, 5); /*set major version to
>>> majorBrandStr - add NULL terminator */
>>>
>> AV_WB32
>>
>>
> Since we are dealing with string (4 characters) I think using the strlcpy is
> more suitable.
> This also inserts the NULL terminator at the end of the destination string.

We are dealing with uint32_t here, and you are reading one byte too much.

 > [...]
 >
> +        if (nextCompBrandPtr[0]&&  nextCompBrandPtr[1]&&  nextCompBrandPtr[2]&&  nextCompBrandPtr[3]) {
> +            memcpy(currCompBrandPtr,&nextCompBrand, 4);
> +            currCompBrandPtr += 4;
> +        }
> +        else {

else placement :)

[...]

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org