GNOME Bugzilla – Bug 410116
banshee shows trailing comma that doesn't exist
Last modified: 2008-07-21 20:06:58 UTC
Banshee shows trailing commas in some track metadata. On those tracks where it shows this "ghost" comma it shows up in both the main track list and in the song metadata dialog. See screenshot.
Created attachment 82984 [details] trailing comma screenshot
Assigning to Brian, maintainer of TagLib# Brian, I've seen this before as well. The times I've seen it though, there are dozens of trailing commas. I can dig up offending files if you'd like.
Didn't mention that all of these files are MP3. None of the other player that I've tried(Rhythmbox, quodlibet, cmus) nor mp3info show the comma.
Created attachment 82990 [details] [review] Remove empty strings Here is my estimation of what's happening: The orginal tagger broke spec and include nul terminators on Id3v2 strings. TagLib# reads nul terminators as string separators via the Id3v2.4 standard. Thus you end up with "Foobar\0" -> {"Foobar", ""}. Banshee then compbines artist strings with "," giving you "Foobar," This is an old problem I guess I never got around to fixing. Try applying this patch in the taglib-sharp directory: "patch -p0 < taglib-sharp-strip-empty-strings.diff" and recompile banshee to see if this solves the problem.
Created attachment 82991 [details] [review] Remove last string if empty. On second thought, this would be less sloppy. Apply this one instead.
I got a file from Timm Preetz which suffers the multicomma problem so I was able to test it. I've committed a patch which removes all empty strings from the end of the text frame. http://svn.myrealbox.com/viewcvs/trunk/taglib-sharp/src/TagLib/Id3v2/Frames/TextIdentificationFrame.cs?rev=72636&r1=73276&r2=72636&makepatch=1&diff_format=h Brett, can you test this out?
Sorry, I'm not setup to compile Banshee at the moment.
I found some files on my computer, downloaded from eMusic, which suffer the same problem. The patch seems to fix them, so I'm going to mark this as fixed. Please reopen this bug if the problem exists in the next release of Banshee.
I'm concerned that this patch will get lost in the shuffle if the bug is closed before the patch is applied so I'll open it, give it a 0.11.x milestone and assign it to myself. Brian, I'm going to remove you from the bug so that you don't have to get more info on something you've taken care of from your perspective. That way it will show up in queries of 0.11.x bugs that haven't been resolved in the code yet and won't be as likely to be missed.
Heh, adding Brian back, I've got a nit-pick :) Brian: please use String.Empty instead of "" as String.Empty is a static copy of "". Each time you use "" you add another object instantiation, which is a perf hit. Also, with performance in mind: Would it be better to walk the list forward and remove the range from the first null/empty tag to the end of the list vs walking backwards and removing one by one until you reach the first non-null? I have some songs where I literally get maybe a hundred extra commas due to the joins on the emtpy tag fields. Some pseudo code: for(int i = 0; i < list.count; i++) { if(list[i] == String.Empty) { list.RemoveRange(i, list.count - 1); break; } } I think that in most cases you'll only end up doing one iteration of that loop and removing a large range vs many iterations of single item removes until you get to the head and find that you just have a single valid item. Of course, RemoveRange may do iterations if in fact such a method exists. It may just be better to create a new list with the valid fields and ditch the bad. for(int i = 0; i < list.count; i++) { if(list[i] == String.Empty) { list = list.CopyRange(0, i - 1); break; } } Either way, I think walking it backwards is a poor performance design.
My main concern with walking forward is the possible but improbable {"String", "", "String"} combination. The very existence of this patch is a break of the Id3v2 standard and I'm wary of breaking things. Looping backward seems like the most reasonable way to avoid unintentional breakage. I would be willing to do something like: for (int i = list.count - 1; i >= 0; i--) if (list[i] != string.Empty) { if (list[i] != String.Empty) { if (COPY_IS_FASTER) list = list.CopyRange (0, i); else list.RemoveRange (i+1, list.count - i - 1); break; } Josiah, Banshee pulls the current TagLib# revision on checkout and that copy is built into the Banshee Release (I'm assuming), so it should be that when the bug was fixed in TagLib# it was ipso facto fixed in banshee SVN and the next banshee release. As for the fact that this is not a Banshee bug so much as a TagLib# bug that was found in Banshee, I really shouldn't be removed. The bug *should* be filed in the TagLib# bugzilla but if it's easier for everyone to work on it here, dienu, I'll work on it here. Really though, the bug has nothing to do with Banshee's code and doesn't really require banshee developers to do any work, so I don't see why this bug shouldn't still be assigned to me.
:-) Sorry for the confusion. I'm passing it back. Thanks for explaining my error.
*** Bug 413615 has been marked as a duplicate of this bug. ***
*** Bug 414661 has been marked as a duplicate of this bug. ***
*** Bug 415991 has been marked as a duplicate of this bug. ***
I was hoping 0.12 would have this bug fixed (since the fix is so trivial). Is there any timeline on when one of the several proposed fixes might be committed?
(In reply to comment #16) It'll happen when someone who is affected cares enough to submit a patch or in some other way encourage others to do it while also providing all the information and testing necessary. (By encourage, I mean reimburse them for the effort.) That said, I think Comment #11 indicates that Brian fixed it in TagLib# and as such it is fixed in the latest version of Banshee. If you are running SVN and are seeing the problem, you would be best advised to provide evidence of this and provide a method to recreate your issue. Either way, perhaps you could provide something to the conversation to help?
(In reply to comment #17) It seems I've offended you with my question. If so, I'm sorry, I just saw that the target was listed as 0.11.x and there are three different proposed solutions (all of which would work it appears) yet 0.12 was released. I have just checked out SVN, compiled it, deleted my database, reimported and I still see the bug. The patch: http://bugzilla.gnome.org/attachment.cgi?id=82991 was not applied to the version of Taglib# that came with my SVN checkout. I don't know if any of the other two suggested fixes were applied somewhere else in the code, but in any case the bug is still present. After checking out the SVN, I applied the above patch. Deleted my db and reimported. It fixes the bug. How can I be of more help?
:-) No offense to me. I'm just trying to draw information out of you. Perhaps that last sentence was poorly written and abrupt. Sorry about that. What you provided is an excellent test of the patch and adds exactly the sort of thing I was hoping for and gives the devs something to comment on. Thank you! Anyone have any idea what information is still needed on this? I think the NEEDINFO tag can be removed now that boykin has tested the patch successfully.
I think I found it: On February 19th, Aaron created a svn tag of TagLib# for banshee at: svn://svn.myrealbox.com/source/tags/taglib-sharp-banshee-0.11.7/src/TagLib Presumably, this is part of the banshee code freeze. This means that no changes to TagLib# are making it into banshee, including this patch (applied on February 21st). I'm assigning this bug to Aaron. If you check out TagLib# from svn://svn.myrealbox.com/source/trunk/taglib-sharp/src/TagLib, drop the directory into banshee/ext/taglib-sharp, and rerun make, it should apply all recent changes.
Hi all, bug still exists in banshee 12.1, so I created a patch, which works excellent for me, also removing multiple commas: In "TextIdentificationFrame.cs" exchange protected internal TextIdentificationFrame (ByteVector data, int offset, FrameHeader h) : base (h) { field_list = new StringList (); text_encoding = StringType.UTF8; ParseFields (FieldData (data, offset)); } by protected internal TextIdentificationFrame (ByteVector data, int offset, FrameHeader h) : base (h) { field_list = new StringList (); text_encoding = StringType.UTF8; ParseFields (FieldData (data, offset)); // Remove trailing strings if empty. (Workaround for defective nul terminated tags.) while (field_list.Count != 0 && field_list [field_list.Count - 1] == "") field_list.RemoveAt (field_list.Count - 1); }
Created attachment 86207 [details] [review] Improved patch, removing multiple trailing strings
Hi there. Just to post another success story using this patch: http://bugzilla.gnome.org/attachment.cgi?id=82991 Applied it, recompiled and everything worked like a charm. ^^ (Banshee 0.12.1 on Gentoo)
https://bugs.launchpad.net/banshee/+bug/99938 Fixed in Ubuntu Feisty
Can you test in 0.13.2 or SVN to see if this problem persists? I'm not seeing it here.
Tested on my Gentoo with Banshee 0.13.2 and TagLib# 2.0.2.0 and the problem is gone.
Thanks for testing. If anyone else notices this bug, feel free to reopen it and tell which version of Banshee and TagLib# you have.
I'm getting this bug again with Banshee 0.99.3 and 2.0.3
After rm -fr ~/.config/banshee* and reimporting the library it's fixed.
Re-closing, as per previous comment. If anyone else notices this bug, feel free to reopen it again and please tell which version of Banshee and TagLib# you have.