[FFmpeg-devel] [Ffmpeg-devel] ID3v2

[Michael Niedermayer]

Hi

On Mon, May 21, 2007 at 08:20:43PM +0200, Andreas ?man wrote:
> Hi
> 
> Michael Niedermayer wrote:
> >Hi
> >
> >cosmetic v1/v2 renaming patch ok
> 
> Not resubmitting that again then.
> 
> >
> >the other patch should be split in id3v2 reading and writing patches
> 
> Done
> 
> >int v=0;
> >while(len--)
> >    v= (v<<7) + (get_byte(s)&0x7F);
> >return v;
> >
> >is more flexible and wont cause a buffer overflow if someone by
> >mistake passes a len>4
> 
> Done
> 
> >if taglen is 0 before this function then len will be -1 here and the 0 will
> >be written outside of the array
> 
> Yep
> 
> >
> >
> >[...]
> >
> >>+    case 1:  /* UTF-16 we cannot handle */
> >>+    case 2:  /* UTF-16BE we cannot handle */
> >>+    default:
> >>+        break;
> >>+    }
> >
> >this is useless
> 
> Apart from an informational perspective yes. Deleted it never the less.
> 
> >
> >
> >shouldnt this skip+return also be done in the default: case above?
> 
> Yeah, probably.
> 
> The id3v2-specification is pretty brain damaged when it comes to
> the 'tag size' field in the header. 'tag size' does not include the
> trailing footer (which is only present in v2.4 if flags & 0x10
> is true).
> 
> quote: (from spec)
> 
>    The ID3v2 tag size is the sum of the byte length of the extended
>    header, the padding and the frames after unsynchronisation. If a
>    footer is present this equals to ('total size' - 20) bytes, otherwise
>    ('total size' - 10) bytes.
> 
> This is IMHO a scholar example of how to NOT design these type of
> things. I more correct design would have the initial 'tag size'
> in the header include _everything_ so a parser could safely
> skip all bytes if it cannot parse the particular version.
> 
> Very well, i believe this new version is at least as good
> as the non-id3v2 mp3.c.
> 

[...]
> +static void id3v2_read_ttag(AVFormatContext *s, int taglen, char *dst, int dstlen)
> +{
> +    char *q;
> +    int len;
> +
> +    if(taglen < 1)
> +      return;

indention is inconsistant


[...]
> +    default:
> +        av_log(s, AV_LOG_INFO, "ID3v2.%d tag skipped, cannot handle version\n", version);
> +        url_fskip(&s->pb, len);
> +        return;
> +    }
> +
> +    if((version == 2 && flags & 0x40) || flags & 0x80) {
> +        av_log(s, AV_LOG_INFO, "ID3v2.%d tag skipped, cannot handle encoding\n", version);
> +        url_fskip(&s->pb, len);

the av_log & url_fskip is duplicated

[...]

> +static void id3v2_write_tag(AVFormatContext *s)
> +{
> +    int totlen = 0;
> +    char tracktxt[10];
> +
> +    if(s->track)
> +        snprintf(tracktxt, sizeof(tracktxt) - 1, "%d", s->track);
> +    else
> +        tracktxt[0] = 0;
> +
> +    if(s->title[0])     totlen += 11 + strlen(s->title);
> +    if(s->author[0])    totlen += 11 + strlen(s->author);
> +    if(s->album[0])     totlen += 11 + strlen(s->album);
> +    if(s->genre[0])     totlen += 11 + strlen(s->genre);
> +    if(s->copyright[0]) totlen += 11 + strlen(s->copyright);
> +    if(tracktxt[0])     totlen += 11 + strlen(tracktxt);
> +    if(!(s->streams[0]->codec->flags & CODEC_FLAG_BITEXACT))
> +        totlen += strlen(LIBAVFORMAT_IDENT) + 11;
> +
> +    if(totlen == 0)
> +        return;
> +
> +    put_be32(&s->pb, MKBETAG('I', 'D', '3', 0x04)); /* ID3v2.4 */
> +    put_byte(&s->pb, 0);
> +    put_byte(&s->pb, 0); /* flags */
> +
> +    id3v2_put_size(s, totlen);
> +
> +    if(s->title[0])     id3v2_put_ttag(s, s->title,     MKBETAG('T', 'I', 'T', '2'));
> +    if(s->author[0])    id3v2_put_ttag(s, s->author,    MKBETAG('T', 'P', 'E', '1'));
> +    if(s->album[0])     id3v2_put_ttag(s, s->album,     MKBETAG('T', 'A', 'L', 'B'));
> +    if(s->genre[0])     id3v2_put_ttag(s, s->genre,     MKBETAG('T', 'C', 'O', 'N'));
> +    if(s->copyright[0]) id3v2_put_ttag(s, s->copyright, MKBETAG('T', 'C', 'O', 'P'));
> +    if(tracktxt[0])     id3v2_put_ttag(s, tracktxt,     MKBETAG('T', 'R', 'C', 'K'));
> +    if(!(s->streams[0]->codec->flags & CODEC_FLAG_BITEXACT))
> +        id3v2_put_ttag(s, LIBAVFORMAT_IDENT,            MKBETAG('T', 'E', 'N', 'C'));

the checks for tracktxt[0] could be replaced by s->track which would avoid
the else at the top


> +}
> +
>  static int mp3_write_header(struct AVFormatContext *s)
>  {
> +    id3v2_write_tag(s);
>      return 0;
>  }

whats the use of this wraper function? you could as well just put
id3v2_write_tag in the AVOutputFormat struct

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070521/a9137bee/attachment.pgp>