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?
"Andreas ?man" <andreas at olebyn.nu> writes:
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.
There's some code duplication. That's not good.
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. -- M?ns Rullg?rd mru at inprovide.com
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.
This had better be secure. Almost every historical implementation of id3v2 was full of vulns. That's why rather than supporting it in the mplayer demuxer I specifically made it skip id3v2. Id3v2 also has some stupid things like being able to store arbitrary binary data that might emulate mpeg startcodes. They used to allow (but not require) it to be escaped to prevent startcode emulation, but now the escaping is deprecated... :( I don't object to a working implementation. Just be sure you get it right.. :) Rich
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
Index: libavformat/mp3.c =================================================================== --- libavformat/mp3.c (revision 6324) +++ libavformat/mp3.c (working copy) [...] @@ -167,7 +167,250 @@ (buf[9] & 0x80) == 0); }
-static void id3_get_string(char *str, int str_size, +static inline unsigned int id3v2_size(const char *buf) +{ + return ((buf[0] & 0x7f) << 21) | ((buf[1] & 0x7f) << 14) | + ((buf[2] & 0x7f) << 7) | (buf[3] & 0x7f); +}
no function which is used only for reading/writing a header should be inline
+ +static void id3v2_read_ttag(AVFormatContext *s, int taglen, char *dst, int dstlen) +{ + int len; + int64_t v; + + taglen--; /* account for encoding type byte */ + dstlen--; /* Leave space for zero terminator */ + + switch(get_byte(&s->pb)) { /* encoding type */ + + case 0: /* ISO-8859-1 */ + len = FFMIN(taglen, dstlen); + get_buffer(&s->pb, dst, len); + dst[len] = 0; + /* Skip over non-read bytes */ + len = FFMAX(0, taglen - dstlen); + if(len) + url_fskip(&s->pb, len); + break; + + case 3: /* UTF-8 */ + while(dstlen > 0) { + GET_UTF8(v, (taglen--, get_byte(&s->pb)), break;) + *dst++ = v; + dstlen--; + } + *dst = 0; + /* Skip over non-read bytes */ + if(taglen > 0) + url_fskip(&s->pb, taglen); + break;
IMHO strings in ffmpeg should either be ASCII or UTF-8 but never something random like ISO-8859-1
+ + case 1: /* UTF-16 we cannot handle */ + case 2: /* UTF-16BE we cannot handle */ + default: + url_fskip(&s->pb, taglen); + }
i suggest that a offset_t end= url_ftell() + taglen; and url_fseek() be used to skip over things instead of having several lines of code in every case to do that
+} + + +/* + * Parser for ID3v2.2 + */
change to doxygen compatible comment
+ +static void id3v2_2_parse(AVFormatContext *s, int len, uint8_t flags) +{ + char tmp[16]; + uint32_t tag; + int tlen, i; + uint16_t tflags; + + if(flags & 0xc0) { + /* We cannot deal with unsynchornization nor compression */
comment -> av_log
+ url_fskip(&s->pb, len); + return; + } + + while(len > 0) { + tag = 0; + for(i = 0; i < 3; i++) + tag = tag << 8 | get_byte(&s->pb);
get_be24();
+ + tmp[0] = 0; + if(get_buffer(&s->pb, tmp + 1, 3) != 3) + return; + tlen = id3v2_size(tmp);
a function which converts 3-4 bytes of a ByteIOContext to the size seeme like a good idea as this is occuring several times
+ + len -= 6 + tlen; + + switch(tag) { + case MKBETAG(0, 'T', 'T', '2'): + id3v2_read_ttag(s, tlen, s->title, sizeof(s->title)); + break; + case MKBETAG(0, 'T', 'P', '1'): + id3v2_read_ttag(s, tlen, s->author, sizeof(s->author)); + break; + case MKBETAG(0, 'T', 'A', 'L'): + id3v2_read_ttag(s, tlen, s->album, sizeof(s->album)); + break; + case MKBETAG(0, 'T', 'C', 'O'): + id3v2_read_ttag(s, tlen, s->genre, sizeof(s->genre)); + break; + case MKBETAG(0, 'T', 'C', 'R'): + id3v2_read_ttag(s, tlen, s->copyright, sizeof(s->copyright)); + break; + case MKBETAG(0, 'T', 'R', 'K'): + id3v2_read_ttag(s, tlen, tmp, sizeof(tmp)); + s->track = atoi(tmp); + break; + default: + url_fskip(&s->pb, tlen); + break; + } + } +} + + +/* + * Parser for ID3v2.[34] + */ + +static void id3v2_34_parse(AVFormatContext *s, int len, uint8_t flags) +{ + char tmp[16]; + uint32_t tag; + int tlen; + uint16_t tflags; + + if(flags & 0x80) { + /* We cannot deal with unsynchornization yet, skip entire id3v2 tag */ + url_fskip(&s->pb, len); + return; + } + + if(flags & 0x40) { + /* Extended header present, just skip over it */ + if(get_buffer(&s->pb, tmp, 4) != 4) + return; + url_fskip(&s->pb, id3v2_size(tmp)); + } + + while(len > 0) { + + tag = get_be32(&s->pb); + + if(get_buffer(&s->pb, tmp, 4) != 4) + return; + tlen = id3v2_size(tmp); + + tflags = get_be16(&s->pb); + len -= 10 + tlen; + + switch(tag) { + case MKBETAG('T', 'I', 'T', '2'): + id3v2_read_ttag(s, tlen, s->title, sizeof(s->title)); + break; + case MKBETAG('T', 'P', 'E', '1'): + id3v2_read_ttag(s, tlen, s->author, sizeof(s->author)); + break; + case MKBETAG('T', 'A', 'L', 'B'): + id3v2_read_ttag(s, tlen, s->album, sizeof(s->album)); + break; + case MKBETAG('T', 'C', 'O', 'N'): + id3v2_read_ttag(s, tlen, s->genre, sizeof(s->genre)); + break; + case MKBETAG('T', 'C', 'O', 'P'): + id3v2_read_ttag(s, tlen, s->copyright, sizeof(s->copyright)); + break; + case MKBETAG('T', 'R', 'C', 'K'): + id3v2_read_ttag(s, tlen, tmp, sizeof(tmp)); + s->track = atoi(tmp); + break; + default: + url_fskip(&s->pb, tlen); + break; + }
this switch() should be merged with the id3v2_2 one
+ } + + if(flags & 0x10) { + /* Footer preset, always 10 bytes, skip over it */ + url_fskip(&s->pb, 10); + } +} + + +static void id3v2_parse(AVFormatContext *s, int len, uint8_t version, uint8_t flags) +{ + switch(version) { + case 2: + id3v2_2_parse(s, len, flags); + break; + case 3 ... 4: + id3v2_34_parse(s, len, flags); + break; + } + +} + +static void id3v2_put_size(AVFormatContext *s, int size) +{ + unsigned char buf[4]; + + + buf[0] = size >> 21 & 0x7f; + buf[1] = size >> 14 & 0x7f; + buf[2] = size >> 7 & 0x7f; + buf[3] = size & 0x7f; + put_buffer(&s->pb, buf, 4);
4 put_byte() would be cleaner
+} + +static void id3v2_put_ttag(AVFormatContext *s, char *string, uint32_t tag) +{ + int len = strlen(string); + put_be32(&s->pb, tag); + id3v2_put_size(s, len + 1); + put_be16(&s->pb, 0); + put_byte(&s->pb, 0); /* ISO-8859-1 */
UTF8 should be default
+ put_buffer(&s->pb, string, len); +} + +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 += strlen(s->title) + 11; + if(s->author[0]) totlen += strlen(s->author) + 11; + if(s->album[0]) totlen += strlen(s->album) + 11; + if(s->genre[0]) totlen += strlen(s->genre) + 11; + if(s->copyright[0]) totlen += strlen(s->copyright) + 11; + if(tracktxt[0]) totlen += strlen(tracktxt) + 11;
the above is almost aligned ... and if you want you could also align the ) + 11; ...
+ + totlen += strlen(LIBAVFORMAT_IDENT) + 11; + + 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')); + id3v2_put_ttag(s, LIBAVFORMAT_IDENT, MKBETAG('T', 'E', 'N', 'C'));
LIBAVFORMAT_IDENT must only be stored if the AVCodecCOntext.flags doesnt contain CODEC_FLAG_BITEXACT otherwise regression tests would break if it changes also if you like you could nicely align the MKBETAG so they are more readable [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
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.
[... snip input code...]
IMHO strings in ffmpeg should either be ASCII or UTF-8 but never something random like ISO-8859-1
[... snip output code ...]
UTF8 should be default
I definitely agree on this. But it rises some more questions which is a bit outside the scope of the id3v2-patch itself. The asf-parser just cuts away the top 8 bits from the 16-bit (UTF-16?) encoded strings. (Which currently works quite well together with the ISO-8859-1 encoding). And i bet there are more places where some more or less random string-conversion happens. So the iconv_() functions come in handy. But i dunno if it's is acceptable that ffmpeg depends on iconv? Perhaps adding a CONFIG_ICONV is the right way to go, and unless it's defined we stick to the current "random" string conversion. Should ffmpeg then use UTF-8 (or plain ASCII when compiled without iconv -support) internally?
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.
[... snip input code...]
IMHO strings in ffmpeg should either be ASCII or UTF-8 but never something random like ISO-8859-1
[... snip output code ...]
UTF8 should be default
I definitely agree on this. But it rises some more questions which is a bit outside the scope of the id3v2-patch itself.
The asf-parser just cuts away the top 8 bits from the 16-bit (UTF-16?) encoded strings. (Which currently works quite well together with the ISO-8859-1 encoding). And i bet there are more places where some more or less random string-conversion happens.
So the iconv_() functions come in handy. But i dunno if it's is acceptable that ffmpeg depends on iconv?
Perhaps adding a CONFIG_ICONV is the right way to go, and unless it's defined we stick to the current "random" string conversion.
I think it would be nice to have the option, but not mandatory.
Should ffmpeg then use UTF-8 (or plain ASCII when compiled without iconv -support) internally?
That would be the solution with less code changes. I implemented that on DrFFMPEG and translate the path to open in file.c to wide chars. It's just one function to add. We used to use iconv in DrDivX until we removed it in favour of the built-in windows function and we gain 400 KB of code... Steve
On Mon, Sep 25, 2006 at 07:53:10PM +0200, Steve Lhomme wrote:
We used to use iconv in DrDivX until we removed it in favour of the built-in windows function and we gain 400 KB of code...
This is due to shit implementation of iconv, not the concept of it. My implementation is 2048 bytes... Rich
Rich Felker wrote:
On Mon, Sep 25, 2006 at 07:53:10PM +0200, Steve Lhomme wrote:
We used to use iconv in DrDivX until we removed it in favour of the built-in windows function and we gain 400 KB of code...
This is due to shit implementation of iconv, not the concept of it. My implementation is 2048 bytes...
Publish it if is ready! (and import it in mplayer =P) lu -- Luca Barbato Gentoo/linux Gentoo/PPC http://dev.gentoo.org/~lu_zero
On Tue, Sep 26, 2006 at 03:01:13AM +0200, Luca Barbato wrote:
Rich Felker wrote:
On Mon, Sep 25, 2006 at 07:53:10PM +0200, Steve Lhomme wrote:
We used to use iconv in DrDivX until we removed it in favour of the built-in windows function and we gain 400 KB of code...
This is due to shit implementation of iconv, not the concept of it. My implementation is 2048 bytes...
Publish it if is ready! (and import it in mplayer =P)
It's in my svn repo. However users of non-utf8-only systems won't like it since it doesn't support converting _to_ legacy codepages, only converting _from_ them. :) The legacy CJK conversion support is only partially implemented (and untested) too, but once that's done the code will still be well under 3k. Anyway this implementation should do a plenty for the needs of a program like ffmpeg, since when writing files you should always use utf-8 or ascii anyway. But it wouldn't work well for MPlayer where some users want to convert the messages to legacy codepages for their terminals. Rich
Hi On Mon, Sep 25, 2006 at 06:09:29PM +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.
[... snip input code...]
IMHO strings in ffmpeg should either be ASCII or UTF-8 but never something random like ISO-8859-1
[... snip output code ...]
UTF8 should be default
I definitely agree on this. But it rises some more questions which is a bit outside the scope of the id3v2-patch itself.
The asf-parser just cuts away the top 8 bits from the 16-bit (UTF-16?) encoded strings. (Which currently works quite well together with the ISO-8859-1 encoding). And i bet there are more places where some more or less random string-conversion happens.
So the iconv_() functions come in handy. But i dunno if it's is acceptable that ffmpeg depends on iconv?
Perhaps adding a CONFIG_ICONV is the right way to go, and unless it's defined we stick to the current "random" string conversion.
Should ffmpeg then use UTF-8 (or plain ASCII when compiled without iconv -support) internally?
for what would we need iconv? for iso8859-1 -> utf8 we just need a 128 entry table to convert the high 128 chars utf16->utf8 / utf8->utf16 also doesnt seem like it would need an external lib [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
On Tue, Sep 26, 2006 at 12:42:07AM +0200, Michael Niedermayer wrote:
for what would we need iconv? for iso8859-1 -> utf8 we just need a 128 entry table to convert the high 128 chars
You need a lookup table to map 128->128, 129->129, 130->130, ..., 255->255?? Last I checked y=x worked just fine for that... Rich
Hi On Mon, Sep 25, 2006 at 08:10:06PM -0400, Rich Felker wrote:
On Tue, Sep 26, 2006 at 12:42:07AM +0200, Michael Niedermayer wrote:
for what would we need iconv? for iso8859-1 -> utf8 we just need a 128 entry table to convert the high 128 chars
You need a lookup table to map 128->128, 129->129, 130->130, ..., 255->255?? Last I checked y=x worked just fine for that...
oops, i didnt know that the first 256 chars of unicode match iso8859-1 so yes of course you are right y=x should be used [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In the past you could go to a library and read, borrow or copy any book Today you'd get arrested for mere telling someone where the library is
On Mon, Sep 25, 2006 at 06:09:29PM +0200, Andreas ?man wrote:
UTF8 should be default
I definitely agree on this. But it rises some more questions which is a bit outside the scope of the id3v2-patch itself.
The asf-parser just cuts away the top 8 bits from the 16-bit (UTF-16?) encoded strings. (Which currently works quite well together with the ISO-8859-1 encoding). And i bet there are more places where some more or less random string-conversion happens.
Then fix it; don't add more of this brokenness just to conform with existing brokenness..
So the iconv_() functions come in handy. But i dunno if it's is acceptable that ffmpeg depends on iconv?
There is no need for iconv as long as everything is a form of unicode. Converting between UTF-8, UTF-16, and UTF-32 is plenty easy. Rich
Rich Felker wrote:
On Mon, Sep 25, 2006 at 06:09:29PM +0200, Andreas ?man wrote:
UTF8 should be default
I definitely agree on this. But it rises some more questions which is a bit outside the scope of the id3v2-patch itself.
The asf-parser just cuts away the top 8 bits from the 16-bit (UTF-16?) encoded strings. (Which currently works quite well together with the ISO-8859-1 encoding). And i bet there are more places where some more or less random string-conversion happens.
Then fix it; don't add more of this brokenness just to conform with existing brokenness..
Well, that was going to be my next task, thats why raising the questions and i think i've got an ideas of what people think in the subject. I'll give it a try and leave the id3v2-stuff behind until i have something that works.
Hello, On Mon, Sep 25, 2006 at 08:08:06PM -0400, Rich Felker wrote:
There is no need for iconv as long as everything is a form of unicode. Converting between UTF-8, UTF-16, and UTF-32 is plenty easy.
And the code for it is already in ffmpeg. GET_UTF8 in libavutil for one direction, the other direction is (IIRC) hidden somewhere in libavformat. Gretings, Reimar D?ffinger
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)
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
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.
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
Hi Michael Niedermayer wrote:
Hi
whats the use of this wraper function? you could as well just put id3v2_write_tag in the AVOutputFormat struct
Hm, when i look at it. Do you think that should be done both for the .mp2 and .mp3 muxers, or just only mp3?
Andreas ?man wrote:
Hi
Michael Niedermayer wrote:
Hi
whats the use of this wraper function? you could as well just put id3v2_write_tag in the AVOutputFormat struct
Hm, when i look at it. Do you think that should be done both for the .mp2 and .mp3 muxers, or just only mp3?
to clarify myself: Perhaps it should skip the entire write of an id3v2 header for mp2-files.
Hi On Mon, May 21, 2007 at 10:09:05PM +0200, Andreas ?man wrote:
Andreas ?man wrote:
Hi
Michael Niedermayer wrote:
Hi
whats the use of this wraper function? you could as well just put id3v2_write_tag in the AVOutputFormat struct
Hm, when i look at it. Do you think that should be done both for the .mp2 and .mp3 muxers, or just only mp3?
to clarify myself: Perhaps it should skip the entire write of an id3v2 header for mp2-files.
hmm, why? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No great genius has ever existed without some touch of madness. -- Aristotle
Michael Niedermayer wrote:
Hi
On Mon, May 21, 2007 at 10:09:05PM +0200, Andreas ?man wrote:
Andreas ?man wrote: Perhaps it should skip the entire write of an id3v2 header for mp2-files.
hmm, why?
Dunno, really.. never mind that. Here are the updates I kept mp3_write_header() rather than changing AVOutputFormat pointers, mostly for name consistency reasons.
Hi On Mon, May 21, 2007 at 10:56:22PM +0200, Andreas ?man wrote:
Michael Niedermayer wrote:
Hi
On Mon, May 21, 2007 at 10:09:05PM +0200, Andreas ?man wrote:
Andreas ?man wrote: Perhaps it should skip the entire write of an id3v2 header for mp2-files.
hmm, why?
Dunno, really.. never mind that.
Here are the updates
I kept mp3_write_header() rather than changing AVOutputFormat pointers, mostly for name consistency reasons.
looks ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
Michael Niedermayer wrote:
Hi
On Mon, May 21, 2007 at 10:56:22PM +0200, Andreas ?man wrote:
Michael Niedermayer wrote:
Hi
On Mon, May 21, 2007 at 10:09:05PM +0200, Andreas ?man wrote:
Andreas ?man wrote: Perhaps it should skip the entire write of an id3v2 header for mp2-files.
hmm, why?
Dunno, really.. never mind that.
Here are the updates
I kept mp3_write_header() rather than changing AVOutputFormat pointers, mostly for name consistency reasons.
looks ok
Both applied Ben -- Purple Labs S.A. www.purplelabs.com
participants (8)
-
andreas＠olebyn.nu -
benoit.fouet＠purplelabs.com -
dalias＠aerifal.cx -
lu_zero＠gentoo.org -
michaelni＠gmx.at -
mru＠inprovide.com -
Reimar.Doeffinger＠stud.uni-karlsruhe.de -
slhomme＠divxcorp.com