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

[haim alon]

Hi,

On Sun, Sep 6, 2009 at 6:26 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de>wrote:

> 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?
>
>
Removed.

> +    char* currCTPtr = CTStr;
>
> camelCase is used only for struct names in FFmpeg.
>
>
Variable names have been improved.


> >      uint32_t type = get_le32(pb);
> > -
> > +
>
> Cosmetics (and trailing whitespace, which are impossible to commit)
>
>
Removed

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


> > +    numCT = (atom.size - 8)/4;
> > +    bnum = numCT;
>
> Those sure aren't good variable names.
>
>
Variable names have been improved.


> > +    if (numCT > maxCtypes) /* maximum num of compatible types */
> > +        numCT = maxCtypes;
>
> Use FFMIN()
> Though there shouldn't be an arbitrary limit anyway IMO.
>
>
Now I'm using av_malloc instead of static allocation. The only limitation is
the number of digits used to represent the actual number of compatible
brands. I set it to 7 digits so the limit is 9,999,999 which seems to be
enough.


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

This is an implementation for passing a variable length list. Each item is 4
character length.
The NULL terminated the string (This is a NULL string termination - not the
character '0')
A compatible brand is composed of 4 characters and should not contain '\0'.
Is there a better way?

I'm attaching an updated patch.

Thanks for the remarks,

Haim.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg.compatible.patch
Type: text/x-patch
Size: 2124 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090907/898fdc47/attachment.bin>