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

[haim alon]

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.



> [...]
>
> --
> 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
>


Patch attached.
Thanks,
Haim.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg.compatible.patch
Type: text/x-patch
Size: 1655 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090922/680cb11d/attachment.bin>