Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: [PATCH] another id3v2 patch (was Re: [PATCH] id3v2 -> overcoming 300 byte limit)
From: Bill Napier (napier_at_pobox.com)
Date: 2002-08-29


On Thu, 29 Aug 2002 00:22:01 +0200, Björn Stenberg <bjorn_at_haxx.se> said:
> Bill Napier wrote:
> Another way would be to have the mpeg thread read the whole file
> (instead of skipping over the id3 tags as it does today) and then
> call id3info with a buffer pointer, buffer len and a file descriptor
> (for setid3v1title()).

> Then we wouldn't need to read the file in small chunks (=more speed)
> and we wouldn't open it twice (=more speed). And, as a nice bonus,
> we'd get the whole id3v2 tag in ram, ready to parse.

I've updated the patch to only open the file once (rather than doing
all the work twice). You may complain that there are 2 extra lseeks
in there (one to seek to the beginning of the file in mp3info and the
other to lseek past the v2 tag in mpeg.c), but if it is really already
in the right position in the file, lseek doesn't really do that much
so I felt it was better to be safe and seek to the right spot than to
just assume that we were already at the right offset.

I didn't change the buffer handling code. Mainly, this was done
because I don't fully understand it (yet) and don't want to break
anything. Even if we were to use the same buffer as the mp3 data
(mp3buf) to read in the v2 tag, you would still need to copy it to
another location (like entry->id3v2data) since it would eventually get
overwritten in the buffer. So you can't get around having at least
that one copy. The number of reads that you would be doing from disk
is the same in both cases since we're reading consecutive sectors from
the disk (and the read() code is smart enough to do the right thing).
The only other penalty that you currently pay are fre extra memcpy's
inside of read() since we're not reading on sector boundries (if you
read() on sector boundries, read() does no caching). We could
certainly change the id3v2 parsing to use a sector size buffer and
read() on sector boundries to remove that extra caching (though you
would be breaking some encapsulation to trade off for speed).

I also didn't make things thread safe. Looking at the code, there is
currently no reason to make mp3info thread safe since it is always
called from mpeg.c.

b

-------- Begin Patch --------
Index: firmware/id3.c
===================================================================
RCS file: /cvsroot/rockbox/firmware/id3.c,v
retrieving revision 1.39
diff -u -b -r1.39 id3.c
--- firmware/id3.c 22 Aug 2002 07:59:31 -0000 1.39
+++ firmware/id3.c 29 Aug 2002 16:32:22 -0000
@@ -49,6 +49,12 @@
                              ((b3 & 0x7F) << (1*7)) | \
                              ((b4 & 0x7F) << (0*7)))
 
+#define BYTES_TO_INT(b1,b2,b3,b4) (((b1 & 0xFF) << (3*8)) | \
+ ((b2 & 0xFF) << (2*8)) | \
+ ((b3 & 0xFF) << (1*8)) | \
+ ((b4 & 0xFF) << (0*8)))
+
+
 /* Table of bitrates for MP3 files, all values in kilo.
  * Indexed by version, layer and value of bit 15-12 in header.
  */
@@ -155,13 +161,15 @@
  * Arguments: file - the MP3 file to scen for a ID3v2 tag
  * entry - the entry to set the title in
  *
- * Returns: true if a title was found and created, else false
+ * Returns: none
  */
 static void setid3v2title(int fd, struct mp3entry *entry)
 {
     unsigned int minframesize;
- int size;
- unsigned int readsize = 0, headerlen;
+ unsigned int framehdrsize;
+ int size = 0;
+ unsigned int totalsize = 0;
+ unsigned int readsize = 0, headerlen = 0;
     char *title = NULL;
     char *artist = NULL;
     char *album = NULL;
@@ -169,7 +177,10 @@
     char header[10];
     unsigned short int version;
     int titlen=0, artistn=0, albumn=0, tracknumn=0;
- char *buffer = entry->id3v2buf;
+ /* Don't want to allocate this on the stack (it's kinda big) */
+ static char buffer[ID3_V2_BUF_SIZE];
+ char* buf_offset = buffer;
+ char* entry_offset = entry->id3v2buf;
         
     /* 10 = headerlength */
     if(entry->id3v2len < 10)
@@ -182,22 +193,54 @@
 
     version = (unsigned short int)header[3];
         
+ /* Set minimun frame size according to ID3v2 version */
+ if(version > 2) {
+ minframesize = 12;
+ framehdrsize = 10;
+ } else {
+ minframesize = 8;
+ framehdrsize = 6;
+ }
+
     /* Read all frames in the tag */
- size = entry->id3v2len - 10;
+ totalsize = entry->id3v2len - 10;
 
- if(size >= (int)sizeof(entry->id3v2buf))
- size = sizeof(entry->id3v2buf)-1;
+ while(totalsize > 0) {
+ int buf_size = ID3_V2_BUF_SIZE;
+ /* Start by adjusting entry_offset to the beginning of the
+ * buffer if we need to */
+ if(size - readsize != 0)
+ {
+ int remaining_size = size - readsize + framehdrsize;
+ int x =0;
 
- if(size != read(fd, buffer, size))
- return;
+ buf_offset = buffer + readsize - framehdrsize;
+ /* Overlapping memcpy */
+ while (x < remaining_size) {
+ *(buffer+x)=*(buf_offset++);
+ x++;
+ }
+ buf_offset = buffer+remaining_size;
+ buf_size -= remaining_size;
+ }
 
- *(buffer + size) = '\0';
+ /* Try and read the whole thing */
+ size = totalsize;
 
- /* Set minimun frame size according to ID3v2 version */
- if(version > 2)
- minframesize = 12;
- else
- minframesize = 8;
+ /* If we can't fit it in the buffer, read what we can */
+ if(size >= buf_size )
+ size = buf_size;
+
+ size = read(fd, buf_offset, size);
+ /* Don't need to check return value here, will be caught
+ * later... (look at the next while loop) */
+
+ /* Take off what we've read from the count */
+ totalsize -= size;
+
+ /* init size and readsize */
+ readsize = 0;
+ size = (buf_offset - buffer) + size;
 
     /*
      * We must have at least minframesize bytes left for the
@@ -209,61 +252,112 @@
         if(version > 2) {
             memcpy(header, (buffer + readsize), 10);
             readsize += 10;
+ if (version > 3) {
             headerlen = UNSYNC(header[4], header[5],
                                header[6], header[7]);
         } else {
+ /* version .3 files don't use synchsafe ints for
+ * size */
+ headerlen = BYTES_TO_INT(header[4], header[5],
+ header[6], header[7]);
+ }
+ } else {
             memcpy(header, (buffer + readsize), 6);
             readsize += 6;
             headerlen = (header[3] << 16) +
                 (header[4] << 8) +
                 (header[5]);
         }
- if(headerlen < 1)
+ /* If we get a 0 headerlen, we're into the padding at the
+ * end of many v2 tags. No need to go through the rest of
+ * the stuff */
+ if(headerlen < 1){
+ /* We don't need to read any more of the v2 tag */
+ totalsize=0;
+ /* Our current buffer is also done */
+ readsize=size;
             continue;
+ }
+
+ /* See if we have the entire frame in the buffer. If
+ * we do, good, keep going. Otherwise, break out of
+ * the loop, set the tags we know, and read in the
+ * next set of info. */
+ if(headerlen > (size - readsize)) {
+ /* This is the last tag in the buffer. We're probably
+ * going to read in some more data on our next time
+ * through. But first we need to sanity check to see
+ * if this frame will *EVER* fit in our buffer (it may
+ * be bigger than our buffer size). In that case, just
+ * skip it*/
+ if (headerlen >= ID3_V2_BUF_SIZE &&
+ headerlen < totalsize) {
+ /* Seek past this XXL frame and adjust all our
+ * counters accordingly */
+ int offset = headerlen - (size - readsize);
+ lseek(fd,offset,SEEK_CUR);
+ totalsize -= offset;
+ readsize=size;
+ }
+ break;
+ }
+
+
         
         /* Check for certain frame headers */
- if(!strncmp(header, "TPE1", strlen("TPE1")) ||
- !strncmp(header, "TP1", strlen("TP1"))) {
+ if(!artist && (
+ !strncmp(header, "TPE1", strlen("TPE1")) ||
+ !strncmp(header, "TP1", strlen("TP1")))) {
             readsize++;
             headerlen--;
- if(headerlen > (size - readsize))
- headerlen = (size - readsize);
- artist = buffer + readsize;
+ memcpy(entry_offset,buffer + readsize,headerlen);
+ artist = entry_offset;
             artistn = headerlen;
             readsize += headerlen;
- }
- else if(!strncmp(header, "TIT2", strlen("TIT2")) ||
- !strncmp(header, "TT2", strlen("TT2"))) {
+ /* Need to pad out headerlen so we can NULL terminate
+ * the string later */
+ entry_offset += headerlen+1;
+ }
+ else if(!title && (
+ !strncmp(header, "TIT2", strlen("TIT2")) ||
+ !strncmp(header, "TT2", strlen("TT2")))) {
             readsize++;
             headerlen--;
- if(headerlen > (size - readsize))
- headerlen = (size - readsize);
- title = buffer + readsize;
+ memcpy(entry_offset,buffer + readsize,headerlen);
+ title = entry_offset;
             titlen = headerlen;
             readsize += headerlen;
+ /* Need to pad out headerlen so we can NULL terminate
+ * the string later */
+ entry_offset += headerlen+1;
         }
- else if(!strncmp(header, "TALB", strlen("TALB"))) {
+ else if(!album &&
+ !strncmp(header, "TALB", strlen("TALB"))) {
             readsize++;
             headerlen--;
- if(headerlen > (size - readsize))
- headerlen = (size - readsize);
- album = buffer + readsize;
+ memcpy(entry_offset,buffer + readsize,headerlen);
+ album = entry_offset;
             albumn = headerlen;
             readsize += headerlen;
+ /* Need to pad out headerlen so we can NULL terminate
+ * the string later */
+ entry_offset += headerlen+1;
         }
- else if(!strncmp(header, "TRCK", strlen("TRCK"))) {
+ else if(!tracknum && !strncmp(header, "TRCK", strlen("TRCK"))) {
             readsize++;
             headerlen--;
- if(headerlen > (size - readsize))
- headerlen = (size - readsize);
- tracknum = buffer + readsize;
+ memcpy(entry_offset,buffer + readsize,headerlen);
+ tracknum = entry_offset;
             tracknumn = headerlen;
             readsize += headerlen;
+ /* Need to pad out headerlen so we can NULL terminate
+ * the string later */
+ entry_offset += headerlen+1;
         } else {
             readsize += headerlen;
         }
     }
-
+ }
     if(artist) {
         entry->artist = artist;
         artist[artistn]=0;
@@ -602,15 +696,15 @@
  * about an MP3 file and updates it's entry accordingly.
  *
  * Arguments: entry - the entry to check and update with the new information
+ * : filename - the name of the file
+ * : fd - the file descriptor to read from
  *
  * Returns: void
  */
-bool mp3info(struct mp3entry *entry, char *filename)
+bool mp3info(struct mp3entry *entry, char* filename, int fd)
 {
- int fd;
- fd = open(filename, O_RDONLY);
- if(-1 == fd)
- return true;
+ /* Got to start from the beginning of the file */
+ lseek(fd,0,SEEK_SET);
 
     memset(entry, 0, sizeof(struct mp3entry));
    
@@ -621,15 +715,13 @@
     entry->id3v2len = getid3v2len(fd);
     entry->tracknum = 0;
 
- if ( entry->id3v2len && entry->id3v2len <= sizeof( entry->id3v2buf ) )
+ if ( entry->id3v2len )
         setid3v2title(fd, entry);
     entry->length = getsonglength(fd, entry);
 
     entry->id3v1len = getid3v1len(fd);
     if(entry->id3v1len && !entry->title)
         setid3v1title(fd, entry);
-
- close(fd);
 
     if(!entry->length || (entry->filesize < 8 ))
         /* no song length or less than 8 bytes is hereby considered to be an
Index: firmware/id3.h
===================================================================
RCS file: /cvsroot/rockbox/firmware/id3.h,v
retrieving revision 1.14
diff -u -b -r1.14 id3.h
--- firmware/id3.h 16 Aug 2002 14:41:47 -0000 1.14
+++ firmware/id3.h 29 Aug 2002 16:32:22 -0000
@@ -21,6 +21,9 @@
 
 #include "file.h"
 
+#define ID3_TOC_SIZE 100
+#define ID3_V2_BUF_SIZE 300
+
 struct mp3entry {
     char path[MAX_PATH];
     char *title;
@@ -42,10 +45,10 @@
     /* Xing VBR fields */
     bool vbr;
     unsigned char vbrflags;
- unsigned char toc[100];/* table of contents */
+ unsigned char toc[ID3_TOC_SIZE];/* table of contents */
 
     /* these following two fields are used for local buffering */
- char id3v2buf[300];
+ char id3v2buf[ID3_V2_BUF_SIZE];
     char id3v1buf[3][32];
 
     /* resume related */
@@ -57,6 +60,6 @@
 #define VBR_BYTES_FLAG 0x02
 #define VBR_TOC_FLAG 0x04
 
-bool mp3info(struct mp3entry *entry, char *filename);
+bool mp3info(struct mp3entry *entry, char* filename, int fd);
 
 #endif
Index: firmware/mpeg.c
===================================================================
RCS file: /cvsroot/rockbox/firmware/mpeg.c,v
retrieving revision 1.114
diff -u -b -r1.114 mpeg.c
--- firmware/mpeg.c 29 Aug 2002 16:23:11 -0000 1.114
+++ firmware/mpeg.c 29 Aug 2002 16:32:25 -0000
@@ -615,7 +615,7 @@
     TSR1 &= ~0x01;
 }
 
-static int add_track_to_tag_list(char *filename)
+static int add_track_to_tag_list(char *filename, int fd)
 {
     struct id3tag *t = NULL;
     int i;
@@ -628,7 +628,7 @@
     {
         /* grab id3 tag of new file and
            remember where in memory it starts */
- if(mp3info(&(t->id3), filename))
+ if(mp3info(&(t->id3), filename, fd))
         {
             DEBUGF("Bad mp3\n");
             return -1;
@@ -671,7 +671,7 @@
         {
             int new_tag_idx = tag_write_idx;
 
- if(add_track_to_tag_list(trackname))
+ if(add_track_to_tag_list(trackname, mpeg_file))
             {
                 /* Bad mp3 file */
                 steps++;
@@ -1311,6 +1311,7 @@
 #ifdef SIMULATOR
     char* trackname;
     int steps=0;
+ int fd = 0;
 
     is_playing = true;
     
@@ -1318,11 +1319,18 @@
         trackname = playlist_peek( steps );
         if (!trackname)
             break;
- if(mp3info(&taginfo, trackname)) {
+ fd = open(trackname, O_RDONLY);
+ if(fd < 0) {
             /* bad mp3, move on */
             steps++;
             continue;
         }
+ if(mp3info(&taginfo, trackname, fd)) {
+ /* bad mp3, move on */
+ steps++;
+ continue;
+ }
+ close(fd);
         playlist_next(steps);
         taginfo.offset = offset;
         set_elapsed(&taginfo);
@@ -1377,15 +1385,23 @@
     char* file;
     int steps = 1;
     int index;
+ int fd =0;
 
     do {
         file = playlist_peek(steps);
         if(!file)
             break;
- if(mp3info(&taginfo, file)) {
+ fd = open(file, O_RDONLY);
+ if(fd < 0) {
+ /* bad mp3, move on */
+ steps++;
+ continue;
+ }
+ if(mp3info(&taginfo, file,fd)) {
             steps++;
             continue;
         }
+ close(fd);
         index = playlist_next(steps);
         current_track_counter++;
         taginfo.index = index;
@@ -1404,15 +1420,23 @@
     char* file;
     int steps = -1;
     int index;
+ int fd = 0;
 
     do {
         file = playlist_peek(steps);
         if(!file)
             break;
- if(mp3info(&taginfo, file)) {
+ fd = open(file, O_RDONLY);
+ if(fd < 0) {
+ /* bad mp3, move on */
+ steps++;
+ continue;
+ }
+ if(mp3info(&taginfo, file, fd)) {
             steps--;
             continue;
         }
+ close(fd);
         index = playlist_next(steps);
         current_track_counter++;
         taginfo.index = index;
Index: uisimulator/common/mpegplay.c
===================================================================
RCS file: /cvsroot/rockbox/uisimulator/common/mpegplay.c,v
retrieving revision 1.7
diff -u -b -r1.7 mpegplay.c
--- uisimulator/common/mpegplay.c 27 May 2002 08:08:54 -0000 1.7
+++ uisimulator/common/mpegplay.c 29 Aug 2002 16:32:26 -0000
@@ -132,18 +132,18 @@
     register signed int s0, s1;
     static struct dither d0, d1;
   
- mp3info(&mp3, fname);
+ if ((fd=open(fname,O_RDONLY)) < 0) {
+ fprintf(stderr,"could not open %s\n",fname);
+ return;
+ }
+
+ mp3info(&mp3, fd, fname);
 
     init_sound(&sound);
 
     /* Configure sound device for this file - always select Stereo because
        some sound cards don't support mono */
     config_sound(&sound,mp3.frequency,2);
-
- if ((fd=open(fname,O_RDONLY)) < 0) {
- fprintf(stderr,"could not open %s\n",fname);
- return;
- }
 
     /* First the structures used by libmad must be initialized. */
     mad_stream_init(&Stream);
-------- End Patch --------

-- 
(defun my-sig()(interactive)(defun c(l)(if(cdddr l)(concat(concat(car l)\" \")
(c(cdr l)))(concat(car l)(cadr l)(caddr l))))(message (c (list \"Just\"
\"another\" emacs-program-name \"hacker\" \"-\" \"napier\" \"@\" \"pobox.com\"))))
(my-sig)



Page was last modified "Jan 10 2012" The Rockbox Crew
aaa