Bugzilla – Bug 6585
Undefined year value can leave wacky values in year field
Last modified: 2009-01-29 09:47:47 UTC
I have a number of songs, mainly from eMusic, which have a TYER tag, but apparently no value in the tag. These seem to cause weird values to show up in the years table in the database, and therefore in the interface. I'm not entirely sure what the contents of these tags are yet, but here's the output from one in id3v2: [root@linux Meander]# id3v2 -l Carbon\ Leaf_Meander_01_Directional.mp3 id3v1 tag info for Carbon Leaf_Meander_01_Directional.mp3: Title : Directional Artist: Carbon Leaf Album : Meander Year: , Genre: Rock (17) Comment: Track: 1 id3v2 tag info for Carbon Leaf_Meander_01_Directional.mp3: TFLT (File type): MPG/3 TIT2 (Title/songname/content description): Directional TPE1 (Lead performer(s)/Soloist(s)): Carbon Leaf TALB (Album/Movie/Show title): Meander TCON (Content type): Rock (17) TYER (Year): TSIZ (Size): 128 TRCK (Track number/Position in set): 1 USER (Terms of use): frame id3v2 only lists the tags which actually exist, so TYER is there. This track shows up in the database with a year of 6678. Not sure if that value stays constant between scans, I would expect it's random and doesn't. The other tracks in the same album are occassionally equally weird. All have a year tag, but it's empty. mysql> select year from tracks where album = 1061; +------+ | year | +------+ | 6678 | | 0 | | 0 | | 0 | | 0 | | 1811 | | 0 | | 1590 | | 2364 | | 0 | | 0 | | 0 | +------+ 12 rows in set (0.00 sec)
Just for good measure: SqueezeCenter Information SqueezeCenter Version: 7.0 - 16145 - Red Hat - EN - utf8 Server IP address: 192.168.0.100 Perl Version: 5.8.8 i386-linux-thread-multi MySQL Version: 5.0.45 Platform Architecture: i686-linux Hostname: linux.koldware.com Server Port Number: 9000 Total Players Recognized: 2 Cache Folder: /var/lib/squeezecenter/cache Plugin Folders: /usr/libexec/Slim/Plugin,/usr/share/squeezecenter/Plugins,/usr/lib/perl5/vendor_perl/Slim/Plugin
I wouldn't always trust the ID3v2 tool to produce sane output based on what's in the ID3v2 tags; it is somewhat strange in my experience. It's handy to attach the header of the MP3 which is causing the program - load the file into some editor and cut out everything after the header. Usually deleting everything after 40K onward gets rid of the MP3 data leaving you with the header which is the thing the problem is about. You can't trust anything but a binary editor to not modify the ID3v2 data, etc, though. As this is a file with a TYER in it, it's obviously an ID3v2.3 file (boo-hiss). There are quite a few paths through lib/MP3/Info.pm which might be followed by short data in the TYER field, and I can't see which might trigger the failure - or how it could get these random values. I'm thinking of an uninitialised $1 being used after a replacement or similar, but I can't see it. Can you trim an example MP3 file down so that I can have a look at the actual problem. If you don't want to trim it and attach here, email it to me (assuming it's less than 6M) at gerph@gerph.org and I'll have a look at it privately (and I can trim and attach it just the pertinent ID3 tag here).
(I'm investigating this now that I've got a file; I'll trim the file and attach it shortly). Oh that's interesting. MP3::Info.pm is generating a 'YEAR' which contains an array reference. And the array contains no items.
Created attachment 2652 [details] Trimmed MP3 file containing the ID3v2.3 frame This is the file that I received from the reporter; I have trimmed it down to remove most of the MP3 data, but left a few frames (including the Xing leading header) as well as the APE trailer.
Whilst I think that the handling of the ARRAY on the TYER is breaking things, the actual problem is one of the decoding of multi-value fields. I think I got this wrong when I wrote that code. I'm going to go and watch telly whilst I eat my tea and think about how to fix this. I think it should be pretty easy, but it is a wider problem than just the TYER - other fields are affected and I would like to fix the lot in one go.
I think what happened with the years was that because an arrayref was being used instead of a bare array, somewhere else in the code (Slim/Formats/MP3.pm maybe) it's doing a match on /(\d\d\d\d)/. The arrayref is a pointer to memory and when represented as a string is a hex address. Thus, sometimes there will be 4 digts together. Other times there won't be. This explains why some are randomly set, and others are 0, I think. Ooh.. Tada. Slim/Formats/MP3.pm, line 267 onward: # We only want a 4-digit year if ( defined $info->{'YEAR'} ) { ($info->{'YEAR'}) =~ s/.*(\d\d\d\d).*/$1/; } I'm relatively convinced. My solution is to strip trailing NULs from the end of the string so that we do not produce an arrayref in this case. The problem is that the TYER was a string of "\x00\x00" which became an ARRAYREF of 3 empty strings. These would then be merged because they're all the same, leaving us with an ARRAYREF which contains just 1 empty string. Unfortunately arrayrefs aren't supported for the YEAR field. Result... breakage. So, in addition to stripping the trailing NULs, I think it's a good idea to also check for an ARRAYREF in the MP3.pm file so that if this happens again (and it will - consider an ID3v1.1 tag with one year and an ID3v2 tag with another, or an ID3v2.4 tag with multi-values on the TDRC field) we can cope. My solution, therefore, affects two files - Slim/Formats/MP3.pm and lib/MP3/Info.pm.
Created attachment 2653 [details] Fix for multi-value ID3v2 TYER fields resulting in random values in the YEAR. Summary: Fix for TYERs with multipe NULs causing random YEARs to be generated. Details: * When a TYER had multiple NULs in it (eg "\x00\x00") this would be converted to an array reference with multiple values in. Because of how these were subsequently processed, the array would become effectively empty. Later, a regular expression in the MP3.pm format processor would try to decode this as a string to find the year and would find an ARRAYREF pointer representation, which it would then use (eg "ARRAY(0x03800A425)" would mean that a year of 380 would be used. The fix here is to : a) strip the trailing NULs from the input data (ensure that we generate a correct data set in the first place) b) check for an ARRAYREF and if we get one, use only the first element. Although we strip trailing NULs from the input, this only tidies the frame a little - an ID3v2.4 frame may still contain multiple frames (eg a TDRC frame with "1998" and "1999" as its values), so part b) is required even if we tidy up the input in a). Implications: Multiple years are discarded using this method. This is not ideal, but it is far better than the current behaviour of using random values. In future it may be useful to support/use multi-value YEAR fields and this will mean further changes to MP3.pm to reflect this, rather than just discarding all but the first. It's also possible that we should only apply the multi-value checks to ID3v2.4 frames. However, the general principle in the MP3::Info.pm library seems to be that if you use 2.3 features in a 2.4 file, they work, and vice-versa - where said features are not mutually exclusive (iTunes frame size problems being an exception to this rule). Thus it would seem reasonable to handle this case. Additionally, there is clearly an ID3v2 writer out there generating these malformed frames - either due to its internal padding requirements, or through carelessness. As it is not leaving a signature, we can only process these frames and hope that the implementation is fixed.
passing this to Chris for review. I'm not sure what need there is for multiple year values, so lets have an official decision. It's also worth finding someone in QA to test this (if the bug has been reproduced). note: it may be too late for 7.0
Patch does seem to fix the problem.
Andy to review patch.
Thanks, patch applied as change 16460.
This bug is being closed since it was resolved for a version which is now released! Please download the new version of SqueezeCenter (formerly SlimServer) at http://www.slimdevices.com/su_downloads.html If you are still seeing this bug, please re-open it and we will consider it for a future release.