Rockbox

could you add a .patch or .diif of the changes?

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.

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.

Let me refer you to my forum post regarding this:

http://forums.rockbox.org/index.php?topic=21757.0


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.

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).

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);
else
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.

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.