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

[Baptiste Coudurier]

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 :>

> +    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.

> +    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 *

> +    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

[...]

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