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

[Baptiste Coudurier]

Hi,

On 09/10/2009 12:55 AM, haim alon wrote:
> Hi,
>
> On Wed, Sep 9, 2009 at 8:47 PM, Baptiste Coudurier<
> baptiste.coudurier at gmail.com>  wrote:
>
>> Hi,
>>
>>
>> On 09/09/2009 06:28 AM, haim alon wrote:
>>
>>> Hi,
>>>
>>> On Wed, Sep 9, 2009 at 3:22 PM, Reimar D?ffinger
>>> <Reimar.Doeffinger at gmx.de>wrote:
>>>
>>>   On Wed, Sep 09, 2009 at 02:00:47PM +0300, haim alon wrote:
>>>>
>>>>> No, at least av_strlcpy reads at most "size-1" bytes, 4 in this case.
>>>>>> The problem is with the return value of it:
>>>>>> return len + strlen(src) - 1;
>>>>>> So my suggestion to replace strncpy with av_strlcpy actually completely
>>>>>> broke things, (av_)strlcpy can only be used for correctly 0-terminated
>>>>>> strings, and I fear that this is used wrongly in some places in
>>>>>> FFmpeg, e.g. rtmppkt.c looks suspicious.
>>>>>>
>>>>>>
>>>>>>   Why there is a problem with the return value?
>>>>> In this case, it copies 4 characters from the source uint32_t and then
>>>>>
>>>> add
>>>>
>>>>> NULL terminator.
>>>>>
>>>> And then it will call strlen on the input value to calculate the return
>>>> value and overread until it either hits a 0 by chance or crashes.
>>>>
>>>>   By the way, I've tried using AV_WB32 instead but it swapped the
>>>>>
>>>> characters.
>>>>
>>>>> Maybe AV_WL32 should be used.
>>>>>
>>>> Since get_le32 was used, yes it should be AV_WL32.
>>>> But the log message will still print it in the wrong order on big-endian
>>>> systems.
>>>> IMO the whole casting mess should be removed and type would better be
>>>> uint8_t type[5] ={0};
>>>> get_buffer(pb, type, 4);
>>>>
>>>>
>>>>   Looks to me like a good solution.
>>>
>>>
>>>   +        // check that char is legal - not NULL
>>>>> +        if (next_comp_brand_ptr[0]&&   next_comp_brand_ptr[1]&&
>>>>>
>>>> next_comp_brand_ptr[2]&&   next_comp_brand_ptr[3]) {
>>>>
>>>> Not sure if this should be checked more tightly, e.g. for ASCII, or for
>>>> printable or...
>>>>
>>>>
>>> I think isprint() is better than just compare to NULL.
>>>
>>
>> IMHO get rid of the cruft and export everything even if it's not printable.
>>
>> "Each brand is a printable four-character code, registered with ISO, that
>> identifies a precise specification."
>>
>> It should not happen, if it happens, well, file is broken and we will add
>> cruft to handle this later.
>>
>
> OK, but the NULL check is important to enable proper dispatching by the
> client application.
>
> Attached.
>
> Thanks,
> Haim.
>
>
> ------------------------------------------------------------------------
>
> Index: libavformat/mov.c
> ===================================================================
> --- libavformat/mov.c	(revision 19796)
> +++ libavformat/mov.c	(working copy)
> @@ -485,15 +485,50 @@
> [...]
>
> +
> +    num_comp_brand = (atom.size - 8) / 4;
> +    comp_brands_str = av_malloc(num_comp_brand*4 + 1); /* 4 characters for each brand + null terminator */
> +    curr_comp_brand_ptr = comp_brands_str;
> +    orig_num_comp_brand = num_comp_brand;
> +    for (i = 0; i<  orig_num_comp_brand; i++) { /*compatible brands*/
> +        next_comp_brand = get_le32(pb);
> +        next_comp_brand_ptr = (char*)&next_comp_brand;
> +        // check that char is legal - not NULL
> +        if (next_comp_brand_ptr[0]&&  next_comp_brand_ptr[1]&&  next_comp_brand_ptr[2]&&  next_comp_brand_ptr[3]) {
> +            memcpy(curr_comp_brand_ptr,&next_comp_brand, 4);
> +            curr_comp_brand_ptr += 4;
> +        } else {
> +            av_log(c->fc, AV_LOG_WARNING, "compatible brand name contains illegal character - skipped.\n");
> +            num_comp_brand -= 1; /* reduce counter since brand is skipped */
> +        }
> +    }
> +    *curr_comp_brand_ptr = '\0';
> +    av_metadata_set(&c->fc->metadata, "compatible_brands", comp_brands_str);
> +    av_freep(&comp_brands_str);
> +    snprintf(num_comp_brand_str, sizeof(num_comp_brand_str), "%d", num_comp_brand);
> +    av_metadata_set(&c->fc->metadata, "compatible_brands_num", num_comp_brand_str);
>       return 0;
>   }

Please remove the special treatment of compatible brands.
It should be one get_buffer only, and compatible_brands_num is useless: 
it will be strlen(compatible_brands)/4.

Besides, as an overall comment, too much code is needed to export 
metadata, but that's not your fault.
Especially these array declarations, snprintf, av_metadata_set for ints 
really annoy me, this should be simplified by adding av_metadata_set_int 
or something.

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