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

[Reimar Döffinger]

On Sun, Sep 06, 2009 at 06:16:33PM +0300, haim alon wrote:
> +/* read major brand, minor version and compatible brands and store them as metadata */
>  static int mov_read_ftyp(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
>  {
> +    const int maxCtypes = 32;
> +    uint32_t mitype,singleCType;
> +    int bnum,numCT,i;
> +    char majorBrandStr[5]; /* 4 characters + null */
> +    char minorVerStr[11]; /* 32 bit integer -> 10 digits + null */
> +    char numCTStr[3]; /* 2 digits + null */                                                          \
> +    char CTStr[maxCtypes*4+1];                                                          \

What are those randomly place \ supposed to be?

> +    char* currCTPtr = CTStr;

camelCase is used only for struct names in FFmpeg.

>      uint32_t type = get_le32(pb);
> -
> +	

Cosmetics (and trailing whitespace, which are impossible to commit)

> +    memcpy(majorBrandStr,&type,4); /*set major version to majorBrandStr*/
> +    majorBrandStr[4] = '\0';

av_strlcpy

> +    numCT = (atom.size - 8)/4;
> +    bnum = numCT;

Those sure aren't good variable names.

> +    if (numCT > maxCtypes) /* maximum num of compatible types */
> +        numCT = maxCtypes;

Use FFMIN()
Though there shouldn't be an arbitrary limit anyway IMO.

> +    for (i=0; i<numCT;i++) { /*compatible brands*/	  
> +        singleCType = get_le32(pb);
> +        memcpy(currCTPtr,&singleCType,4);
> +        currCTPtr += 4;
> +    }
> +    *currCTPtr = '\0';
> +    av_metadata_set(&c->fc->metadata, "CompatibleBrands", CTStr);

That's a rather ugly way to represent it.
Also if for some reason one "compatible brand" has a 0 in it all after
it would just be lost.