Hi,
On 09/22/2009 02:23 AM, haim alon wrote:
> Hi,
>
> On Tue, Sep 22, 2009 at 10:28 AM, Baptiste Coudurier<
> baptiste.coudurier at gmail.com> wrote:
>
>> Hi,
>>
>> On Tue, Sep 22, 2009 at 09:45:32AM +0300, haim alon wrote:
>>> Hi,
>>>
>>> On Thu, Sep 17, 2009 at 8:53 PM, Baptiste Coudurier<
>>> baptiste.coudurier at gmail.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> ...
>>>
>>>
>>>> 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.
>>>>
>>>>
>>> OK, one call to get_buffer to get the compatible brands list (no
>>> verification) and there is no need to deliver compatible_brands_num.
>>>
>>>
>>>
>>>> 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
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at mplayerhq.hu
>>>> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>>>>
>>>
>>> Update attached.
>>>
>>> Thanks,
>>> Haim.
>>
>>> Index: libavformat/mov.c
>>> ===================================================================
>>> --- libavformat/mov.c (revision 19950)
>>> +++ libavformat/mov.c (working copy)
>>> @@ -490,15 +490,32 @@
>>> return 0; /* now go for moov */
>>> }
>>>
>>> +/* read major brand, minor version and compatible brands and store them
>> as metadata */
>>> static int mov_read_ftyp(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
>>> {
>>> - uint32_t type = get_le32(pb);
>>> + uint32_t minor_ver;
>>> + int num_comp_brand;
>>> + char major_brand_str[5]; /* 4 characters + null */
>>> + char minor_ver_str[11]; /* 32 bit integer -> 10 digits + null */
>>> + char* comp_brands_str;
>>> + uint8_t type[5] = {0};
>>
>> An empty line here wouldn't hurt :>
>>
>>
> Sure.
>
>
>>> + get_buffer(pb, type, 4);
>>>
>>> - if (type != MKTAG('q','t',' ',' '))
>>> + if (strcmp(type, "qt, "))
>>> 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);
>>
>> Cosmetics.
>>
>>
> Is it the av_log ?
>
>
>>> + av_strlcpy(major_brand_str, type, 5); /*set major version to
>> major_brand_str */
>>> + av_metadata_set(&c->fc->metadata, "major_brand", major_brand_str);
>>> + minor_ver = get_be32(pb); /*minor version*/
>>
>> Spaces after and before *
>>
>> OK
>
>
>>> + snprintf(minor_ver_str, sizeof(minor_ver_str), "%d", minor_ver);
>>> + av_metadata_set(&c->fc->metadata, "minor_version", minor_ver_str);
>>> +
>>> + num_comp_brand = (atom.size - 8) / 4;
>>> + comp_brands_str = av_malloc(num_comp_brand*4 + 1); /* 4 characters
>> for each brand + null terminator */
>>> + get_buffer(pb, comp_brands_str, num_comp_brand*4);
>>> + comp_brands_str[num_comp_brand*4] = '\0';
>>
>> length of comp_brands_str is atom.size - 8
>>
>>
> I'll use comp_brand_size instead of num_comp_brand.
>
> [...]
>
> Index: libavformat/mov.c
> ===================================================================
> --- libavformat/mov.c (revision 19958)
> +++ libavformat/mov.c (working copy)
> @@ -490,15 +490,32 @@
> return 0; /* now go for moov */
> }
>
> +/* read major brand, minor version and compatible brands and store them as metadata */
> static int mov_read_ftyp(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
> {
> - uint32_t type = get_le32(pb);
> + uint32_t minor_ver;
> + int comp_brand_size;
> + char major_brand_str[5]; /* 4 characters + null */
> + char minor_ver_str[11]; /* 32 bit integer -> 10 digits + null */
> + char* comp_brands_str;
> + uint8_t type[5] = {0};
>
> - if (type != MKTAG('q','t',' ',' '))
> + get_buffer(pb, type, 4);
> + if (strcmp(type, "qt, "))
> 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_strlcpy(major_brand_str, type, 5); /* set major version to major_brand_str */
> + av_metadata_set(&c->fc->metadata, "major_brand", major_brand_str);
You don't need the copy and major_brand_str, use type.
> + minor_ver = get_be32(pb); /* minor version */
> + snprintf(minor_ver_str, sizeof(minor_ver_str), "%d", minor_ver);
> + av_metadata_set(&c->fc->metadata, "minor_version", minor_ver_str);
> +
> + comp_brand_size = atom.size - 8;
> + comp_brands_str = av_malloc(comp_brand_size + 1); /* Add null terminator */
Check for malloc return.
[...]
--
Baptiste COUDURIER
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org