Bugzilla – Bug 3972
Null-delimited lists in ID3v2.4 text frames
Last modified: 2008-12-18 11:12:12 UTC
The ID3v2.4 informal standard (Native Frames document, section 4.2: http://www.id3.org/id3v2.4.0-frames.txt) indicates that, while text information frames must not be duplicated, null-delimited lists may be used to include multiple strings within a single frame. SlimServer currently supports this for GENRE (TCON), but not for other frames such as ARTIST (TPE1). I feel the same logic should apply to other text fields. A potential solution has been proposed by gerph: http://forums.slimdevices.com/attachment.php?attachmentid=1585&d=1156015617. While the use of null separators in this context may seem obscure (only foobar2000 and mp3tag support it AFAIK), it offers the following advantages over text separators (such as '/' or ';'): 1. Graceful degradation to ID3v1. Only the first string in the null-separated list is written to the ID3v1 tag, reducing clutter (and its repercussions: split-up albums, i.e.) in legacy devices. 2. Smooth integration integration with foobar2000, a popular audio player for Windows. 3. Improved integration (or graceful degradation) with MusicMagic/MusicIP, which does not support text separators but ignores strings following the first null. (Currently, values in a text-delimited list appear 'mixable' from SlimServer but are not recognized by the MIP API, resulting in an empty mix. Trailing artists--for instance--in a null separated list will not appear mixable to begin with. That's how it works with Vorbis comments.) I recognize that implementing this could be complex; thanks for considering it.
Created attachment 1453 [details] MP3 demonstrating null-delimited TPE1 in ID3v2.4 tag
We are quite close to shipping 6.5, so I have to mark this as 'future' for now.
Is it appropriate to request a target milestone for this one now that 6.5.0 is out? I've been using gerph's patch for the past 6 weeks or so--stable, no problems with scanning MP3 files.
Perhaps Dan has an opinion on whether this can be added to 7.0.
Created attachment 1711 [details] Patch for multi-value T??? fields. This patch is the most recent of the changes that I've made to the MP3::Info processor. It is intended to ensure that fields with multiple values in them in 2.4 frames are supplied within an array to the requester.
Created attachment 1721 [details] Multi-text field support This is a diff against revision 10736. It provides support for... ... multiple nul-separated text fields as required by ID3v2.4. ... improved merging of duplicate tags. Not used by SlimServer. If the request is for both ID3v1 and ID3v2.4 then the fields may end up with nuls terminating the strings which causes the fields to differ and not be merged correctly - thus it may seem that the artist appears twice, once for ID3v1 and once for ID3v2.x. This is corrected.
*** Bug 4623 has been marked as a duplicate of this bug. ***
Just a note to say I've been using gerph's patch for about 8 months with no ill effects. Works like a charm, actually. Are there any barriers to implementing it into SlimServer? --fb2k and mp3tag use id3v2.4 tags & null-delimited fields --this practice is standards-compliant --id3v2.4, admittedly a late bloomer, is gaining momentum --gerph has done all the heavy lifting; the patch is already done --and tested on a diverse library
(In reply to comment #8) > --id3v2.4, admittedly a late bloomer, is gaining momentum *finally* :-) > --gerph has done all the heavy lifting; the patch is already done > --and tested on a diverse library I just wanted to say that whilst I appreciate the sentiment, a lot of the work had already been done to make multi-values work in the core design of SlimServer and I've merely utilised that facility with the ID3v2.4 multi-value frames. It would be nice to see it go in; sadly I've not upgraded my own slimserver here, so I can't check whether the patches work with a modern version. At some point I will upgrade to the more recent version and hopefully will be able to update my patches appropriately.
This should be classified as a bug since SlimServer fails to meet the specification. Maybe if someone at Slim Devices one day starts working again on scanning and database problems, then this fix could be considered.
I agree, Jim. I'll make an executive decision that we should put this into 7, since 6.5.2 is out the door today (crossing fingers).
I'd like to suggest that if Justin produces a patch for the 6.5.2 release and it passes preliminary user testing, that this could be targeted for 6.5.3.
(In reply to comment #12) > I'd like to suggest that if Justin produces a patch for the 6.5.2 release and > it passes preliminary user testing, that this could be targeted for 6.5.3. You can suggest it, and I'll try my best to get a patch up in the next week or so. I've not actually got 6.5.2 installed here; I'm using one of the beta 7's from a while back at the moment. Shouldn't be hard to install and create a patch for, though.
I've just used the most recent patch (2006-11-20) on lib/MP3/Info.pm in 6.5.2 and it applies straight off with no changes. So, it could be applied without any problems. I'll run it in 6.5.2 for a bit and see whether there are any problems, but I don't expect any as it's been running in the pre-7.0 version I've been running for some time (and windowshade's been using it for longer, but on a pre-6.5.2 version).
(In reply to comment #14) > I've been running for some time (and windowshade's been using it for longer, > but on a pre-6.5.2 version). > I've been using 6.5.2 since early March 2007--absolutely no trouble with your patch. (And I've run about a thousand wipes/rescans playing with bug 4882.)
Created attachment 2028 [details] trunk version trunk version only requires minor changes to one of the sections so that it will apply. As this is a CPAN module, the patches should be forwarded as well
I'm ready to start testing this for the 6.5.3 release if someone will put it in the branch.
added to 6.5.3 at change 12151, and trunk at change 12152 feel free to add this to the specs and test away. reopen if there are any problems.
I belive the patch included here is causing bug 5166 which can cause the server to crash. Please could the author of this patch validate the patch I've added to that bug. The problem is that for the problem files this patch returns attribute arrays of the form: 'ARTIST' => [ 'Dr Lecter', [ 'The Police' ] ], This causes the sql code to croak...
Eek; that sounds very bad. I don't really have the time to debug that now, but it doesn't seem too unbelievable that that would happen - surprising that it didn't get shown up in testing, though :-(
Turns out it's not a complicated fix and checking the patch in bug 5166 is pretty easy. That patch does appear to fix it. Really sorry that my code didn't work straight off :-(
If you examine the mp3s in the other bug report that break the scanner, the problem turned up in id3v2.3 tags when there are two frames of the same name in the tag. Apparently, some ripping or tagging program(s) are doing that. So probably not surprising that it didn't turn up in testing with well-behaved files.
Curse me and my well-formed files! :-|
(In reply to comment #23) > Curse me and my well-formed files! :-| > Guilty as charged. Next time I test a patch, remind me to scan some illegal downloads or something. My hat is off to developers like Justin and Adrian--I don't know how you keep on top of everything (we) users might try!
(In reply to comment #24) > (In reply to comment #23) > > Curse me and my well-formed files! :-| > > > > Guilty as charged. Next time I test a patch, remind me to scan some illegal > downloads or something. My hat is off to developers like Justin and Adrian--I > don't know how you keep on top of everything (we) users might try! Nah; don't thank me - I'm the one that broke it and caused people pain trying when it crashed and in trying to debug. I should have probably tried malformed files but the multi-tagged files never even occurred to me when the specification stated explicitly what to do for multiple items... Mea culpa.