[FFmpeg-devel] [Ffmpeg-devel] ID3v2

[Michael Niedermayer]

Hi

On Sat, May 19, 2007 at 08:24:19PM +0200, Andreas ?man wrote:
> Hi
> 
> Michael Niedermayer wrote:
> >Hi
> >
> >On Sun, Sep 24, 2006 at 01:21:40PM +0200, Andreas ?man wrote:
> >>Hi
> >>
> >>I've written a tentative patch that adds ID3v2.[234] reading
> >>and ID3v2.4 writing to the mp3 (de)muxer. I've tested it
> >>together with xmms and itunes.
> >>I'd say the primary reasons for using ID3v2 is:
> >>
> >>* It's located at the beginning of the file
> >>  (obviously a benefit when streaming)
> >>* It supports strings > 30 chars
> >>
> >>Comments welcome.
> >>
> >>While at it, I renamed some stuff to better reflect what
> >>ID3-version it is used for.
> >>Perhaps I should submit that as a separate patch?
> >
> >yes, that has to be a seperate patch
> >
> 
> This was a while ago, but i've revived this patch and fixed
> according to your comments. The code now assumes that
> the strings in avformatcontext are utf8 encoded (which
> other muxers seems to do now as well)
> 
> 

cosmetic v1/v2 renaming patch ok

the other patch should be split in id3v2 reading and writing patches


[...]
> +static unsigned int id3v2_get_size(ByteIOContext *s, int len)
> +{
> +    char tmp[16];
> +    tmp[0] = 0;
> +    if(get_buffer(s, tmp + 4 - len, len) != len)
> +        return 0;
> +    return id3v2_size(tmp);
> +}

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


> +
> +static void id3v2_read_ttag(AVFormatContext *s, int taglen, char *dst, int dstlen)
> +{
> +    char *q;
> +    int len;
> +
> +    taglen--; /* account for encoding type byte */
> +    dstlen--; /* Leave space for zero terminator */
> +
> +    switch(get_byte(&s->pb)) { /* encoding type */
> +
> +    case 0:  /* ISO-8859-1 (0 - 255 maps directly into unicode) */
> +        q = dst;
> +        while(taglen--) {
> +            uint8_t tmp;
> +            PUT_UTF8(get_byte(&s->pb), tmp, if (q - dst < dstlen - 1) *q++ = tmp;)
> +        }
> +        *q = '\0';
> +        break;
> +
> +    case 3:  /* UTF-8 */
> +        len = FFMIN(taglen, dstlen);
> +        get_buffer(&s->pb, dst, len);
> +        dst[len] = 0;

if taglen is 0 before this function then len will be -1 here and the 0 will
be written outside of the array


[...]

> +    case 1:  /* UTF-16 we cannot handle */
> +    case 2:  /* UTF-16BE we cannot handle */
> +    default:
> +        break;
> +    }

this is useless


> +}
> +
> +/**
> + * ID3v2 parser
> + */
> +
> +static void id3v2_parse(AVFormatContext *s, int len, uint8_t version, uint8_t flags)
> +{
> +    int isv34, tlen;
> +    uint32_t tag;
> +    offset_t next;
> +    char tmp[16];
> +    int taghdrlen;
> +
> +    switch(version) {
> +    case 2:        isv34 = 0;  break;
> +    case 3 ... 4:  isv34 = 1;  break;
> +    default:                   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);
> +        return;
> +    }

shouldnt this skip+return also be done in the default: case above?


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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20070520/becdfa95/attachment.pgp>