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

[haim alon]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg.compatible.patch
Type: text/x-patch
Size: 2801 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090910/662c42b6/attachment.bin>