FS#10240 - Rockbox not handling MP3s with large ID3 frames

Attached to Project: Rockbox
Opened by Rhino Banga (rhinobanga) - Monday, 25 May 2009, 10:04 GMT
Last edited by Andree Buschmann (Buschel) - Sunday, 30 January 2011, 20:52 GMT
Task Type Bugs
Category ID3 / meta data
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.2
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


Whilst RB skips forward past large ID3 frames it doesn't seem to reset it's use of the internal buffer.

This is causing the internal scanning loop to exit early, causing it to miss relevant ID3 tags.

In mp3.c:setid3v2title changing this:

/* Seek to the next frame */
if(framelen < totframelen)
lseek(fd, totframelen - framelen, SEEK_CUR);

To this:

/* Seek to the next frame */
if(framelen < totframelen)
lseek(fd, totframelen - framelen, SEEK_CUR);
bufferpos = 0;

Cured the problem for me and my entire MP3 collection (6000+ tracks) was then catalogued properly on my 80gb iPod.

Personally though I think that entire function could do with a re-write to make better use of the buffer.
This task depends upon

Closed by  Andree Buschmann (Buschel)
Sunday, 30 January 2011, 20:52 GMT
Reason for closing:  Fixed
Additional comments about closing:  The underlying issue (incomplete parsing due to early abort when buffer is filled) is fixed with r29174.
Comment by Seth (froggyman) - Tuesday, 26 May 2009, 01:39 GMT
could you add a .patch or .diif of the changes?
Comment by Rhino Banga (rhinobanga) - Tuesday, 26 May 2009, 17:21 GMT
Not easily as I just wiped my VM image (in the process of moving to OpenSUSE 11.1) ... anyway it 's just that change in the details. I made no other modification. So it should be very easy for someone to replicate.
Comment by Magnus Holmgren (learman) - Saturday, 30 May 2009, 09:48 GMT
bufferpos should not be reset for big frames. If the scanning loop exits early, it is because the tag buffer is full. Resetting bufferpos would just mean overwriting previously read tag data. In 3.2, the buffer was pretty small, only 300 bytes. It has been increased to 900 bytes, but that might not be enough.
Comment by Rhino Banga (rhinobanga) - Saturday, 30 May 2009, 19:58 GMT
Let me refer you to my forum post regarding this:

I would suggest you throw a relevant MP3 file at this code, to see it's behaviour and why the above reset worked around the problem.
Comment by Magnus Holmgren (learman) - Sunday, 31 May 2009, 09:15 GMT
And I would suggest you take a look at the "Show track info" screen and look at the "Comment" field for one of those files. :) It will probably be a copy of one of the other fields, which isn't what is intended. It's just luck that it works as well as it does for you (i.e., luck that the large COMM frame is first).

In the post you said this: "Which got me thinking, if the code seeks to a new position then the internal buffer and buffer position must be invalid but the buffer position is not being reset." If the code seeks to a new position, then the frame was larger than the remaining buffer space, so the frame wasn't fully read. Still, the buffer might not be completely full at this point (e.g., the frame was in Unicode format but didn't contain many non-ASCII chars), so it's worth trying to read more.

A better work-around for you would be to remove the COMM field from the taglist, so it isn't read, filling up the tag buffer (id3v2buf in mp3entry).
Comment by Rhino Banga (rhinobanga) - Sunday, 31 May 2009, 09:43 GMT
Hi Magnus, I appreciate the time you are taking on this and please don't take this the wrong way but I don't think telling people to change their perfectly fine tags is the solution compared to fixing RB. Isn't that the whole point of open-source?

Anyway, after the lseek happens the code jumps to the while loop:

while (size >= minframesize && bufferpos < buffersize - 1) {

The code following that doesn't use the buffer as a read-ahead cache, but instead uses it as a temporary buffer, e.g.:

/* found a tag matching one in tagList, and not yet filled */
tag = buffer + bufferpos;

if(global_unsynch && version <= ID3_VER_2_3)
bytesread = read_unsynched(fd, tag, framelen);
bytesread = read(fd, tag, framelen);

So can you explain again why is it inappropriate to reset buffer position so a) it gets past the while loop and b) it uses as much of the temp area buffer as possible.

Comment by Magnus Holmgren (learman) - Sunday, 31 May 2009, 14:24 GMT
I'm certainly not against changing the code as such. The question is, how should it be done to fix this case, without breaking others?

And when I said "remove the COMM field from the taglist" I referred to the "struct tag_resolver taglist" in metadata/mp3.c. No changes to the tags needed. :) Not a good long-term solution perhaps, but it would solve your immediate problem.

As for the code, what you need to remember is that buffer always points to entry->id3v2buf. If a frame is supported (i.e., is in the taglist), the data of the frame is read into (a part of) buffer. If the read data is to be kept, then bufferpos is updated, and a pointer in entry is set to somewhere in id3v2buf. So buffer (id3v2buf) is not a temporary buffer, it is the area used to store the frame data for later use. Therefore you can't reset bufferpos and expect things to work properly. In this case, entry->comment, which fills the entire buffer, would be overwritten by other tags.