[FFmpeg-devel] [PATCH] av_metadata_count()
Aurelien Jacobs
aurel
Mon Feb 2 01:49:23 CET 2009
Michael Niedermayer wrote:
> On Sun, Feb 01, 2009 at 10:11:29PM +0100, Aurelien Jacobs wrote:
> > Michael Niedermayer wrote:
> >
> > > On Sun, Feb 01, 2009 at 06:36:44PM +0100, Aurelien Jacobs wrote:
> > > > On Sun, 1 Feb 2009 02:06:21 +0100
> > > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > >
> > > > > On Sun, Feb 01, 2009 at 02:00:12AM +0100, Aurelien Jacobs wrote:
> > > > > > On Fri, 30 Jan 2009 17:33:02 +0100
> > > > > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > > >
> > > > > > > On Fri, Jan 30, 2009 at 01:43:34AM +0100, Aurelien Jacobs wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Attached patch adds a new metadata API func to get the number of
> > > > > > > > available tags among a list of keys. This func will be useful
> > > > > > > > at least for the following muxers: asf, mov, mp3.
> > > > > > > > It can also be quite handy for a user application.
> > > > > > >
> > > > > > > i cant see this to be that usefull, please first show the code
> > > > > > > that would benefit from this.
> > > > > >
> > > > > > OK. As an example, here is a snipet from movenc:
> > > > > > // only save meta tag if required
> > > > > > if (s->title[0] || s->author[0] || s->album[0] || s->year ||
> > > > > > s->comment[0] || s->genre[0] || s->track) {
> > > > > > It could be replaced by something very ugly:
> > > > > > // only save meta tag if required
> > > > > > if (av_metadata_get(s->metadata, "title" , NULL, 0) ||
> > > > > > av_metadata_get(s->metadata, "author" , NULL, 0) ||
> > > > > > av_metadata_get(s->metadata, "album" , NULL, 0) ||
> > > > > > av_metadata_get(s->metadata, "year" , NULL, 0) ||
> > > > > > av_metadata_get(s->metadata, "comment", NULL, 0) ||
> > > > > > av_metadata_get(s->metadata, "genre" , NULL, 0) ||
> > > > > > av_metadata_get(s->metadata, "track" , NULL, 0)) {
> > > > > > Or by something simpler like:
> > > > > > static const char const *mov_tags[] = {
> > > > > > "title", "author", "album", "year", "copyright", "comment",
> > > > > > "genre", "track", NULL
> > > > > > };
> > > > > > // only save meta tag if required
> > > > > > if (av_metadata_count(s->metadata, mov_tags, 0)) {
> > > > > >
> > > > > > We have something similar in the mp3 muxer, except that it currently
> > > > > > only check the presence of title to deceide whether it should mux
> > > > > > metadata or not (this is wrong and it should really check for the
> > > > > > presence of any supported tag).
> > > > > > Attached is a patch to use new metadata API in mp3 (de)muxer, which
> > > > > > make use of av_metadata_count(). (I have not yet finalized patch
> > > > > > for the mov muxer, but it will use something similar, twice)
> > > > >
> > > > > url_open_dyn_buf(&bc)
> > > > > write title
> > > > > write author
> > > > > ...
> > > > >
> > > > > if bc is not empty write it out
> > > >
> > > > That's not as simple as "bc is not empty", because some data is written
> > > > in the metadata header anyway, so bc won't ever be empty. But still,
> > > > it's possible to do something like this by counting the number of tag
> > > > which are actually written in the buffer.
> > > >
> > > > So attached is a new patch to upgrade the mp3 (de)muxer to the new
> > > > metadata API without using av_metadata_count().
> > > >
> > > > Aurel
> > >
> > > > Index: libavformat/mp3.c
> > > > ===================================================================
> > > > --- libavformat/mp3.c (revision 16909)
> > > > +++ libavformat/mp3.c (working copy)
> > > > @@ -167,18 +167,15 @@
> > > > return v;
> > > > }
> > > >
> > > > -static void id3v2_read_ttag(AVFormatContext *s, int taglen, char *dst, int dstlen)
> > > > +static void id3v2_read_ttag(AVFormatContext *s, int taglen, const char *key)
> > > > {
> > > > - char *q;
> > > > - int len;
> > > > + char *q, dst[512];
> > > > + int genre, len, dstlen = sizeof(dst)-1;
> > > >
> > > > - if(dstlen > 0)
> > > > - dst[0]= 0;
> > > > if(taglen < 1)
> > > > return;
> > > >
> > > > taglen--; /* account for encoding type byte */
> > > > - dstlen--; /* Leave space for zero terminator */
> > > >
> > > > switch(get_byte(s->pb)) { /* encoding type */
> > > >
> > > > @@ -196,7 +193,18 @@
> > > > get_buffer(s->pb, dst, len);
> > > > dst[len] = 0;
> > > > break;
> > > > +
> > > > + default:
> > > > + dst[0] = 0;
> > > > + break;
> > > > }
> > > > +
> > >
> > > > + if (!strcmp(key, "genre") &&
> > > > + sscanf(dst, "(%d)", &genre) == 1 && genre <= ID3v1_GENRE_MAX)
> > > > + strcpy(dst, id3v1_genre_str[genre]);
> > >
> > > sechole
> >
> > Oh, indeed. A negative genre could lead to a buffer overrun. Fixed.
> > The strcpy itself should be safe as all the strings in id3v1_genre_str[]
> > are less than 20 chars while dst is 512 bytes. But in case something
> > change in the future, I've now used av_strlcpy instead, for extra safety.
> >
> > Aurel
> > Index: libavformat/mp3.c
> > ===================================================================
> > --- libavformat/mp3.c (revision 16921)
> > +++ libavformat/mp3.c (working copy)
> > @@ -167,18 +167,16 @@
> > return v;
> > }
> >
> > -static void id3v2_read_ttag(AVFormatContext *s, int taglen, char *dst, int dstlen)
> > +static void id3v2_read_ttag(AVFormatContext *s, int taglen, const char *key)
> > {
> > - char *q;
> > - int len;
> > + char *q, dst[512];
>
>
> > + int len, dstlen = sizeof(dst)-1;
> [...]
> >
> > - if(dstlen > 0)
> > - dst[0]= 0;
> > if(taglen < 1)
> > return;
> >
> > taglen--; /* account for encoding type byte */
> > - dstlen--; /* Leave space for zero terminator */
> >
> > switch(get_byte(s->pb)) { /* encoding type */
> >
> > @@ -196,7 +194,18 @@
> > get_buffer(s->pb, dst, len);
> > dst[len] = 0;
> > break;
> > +
> > + default:
> > + dst[0] = 0;
> > + break;
> > }
>
> what is the relation between this change and metadata support?
> I dont mind at all if you simplify or chaneg it but in a seperate patch
Well, it was a simplification that was made possible by using the new
API, but it could indeed be applied later. So removed for now.
> also the muxer and demuxer side could be split
I didn't thought it would be useful, but here it is.
Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: md_mp3_dec.diff
Type: text/x-patch
Size: 4282 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090202/fc5f79ce/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: md_mp3_enc.diff
Type: text/x-patch
Size: 5522 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090202/fc5f79ce/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list