Bugzilla – Bug 1855
incorrect cue sheet interpretation
Last modified: 2009-09-08 09:30:22 UTC
A wav file with an associated cue sheet which holds a END statement for each track (including the last one) does not play. The album and track appear in the SlimServer listings, but the tracks don't have any "length". The d_parse log file then includes error messages of the type "parse: Couldn't process anchored file fragment for ...". Removing the END statement of the last track in the cue sheet, makes the problem disappear. A possible source of the problem is this: In SlimServer, the code in Parse.pm reads the cue file with sub readCUE, which calls parseCUE and later on processAnchor. This latter, processAnchor, will check whether $attributesHash->{'SECS'} has a value, which can only have been generated by the line 314: my $track = $ds->updateOrCreate({ 'url' => $filename, 'readTags' => 1, }); However, this line gets only called when at least the last track in a cue sheet doesn't have an END statement and the length of the wav file needs to be read from that file itself.
please attach the cue sheet, and let us know which application generated the CUE sheet. I suspect this was caused by the fix for bug 656
the WAV file might also be useful. Probably too large to attach here, but if you can make it available online for a short time, that would help.
Michael - any thoughts on this? Or is it just a regression? Thanks.
my test cuesheet has an END statement on some of the middle tracks instead but not on the last track. I'll change that and see if I can come up with a fix.
Created attachment 662 [details] cue file which allows to reproduce the bug Attached is a simple cue file. The last line is the one that causes the problem. The way it is now, the last line will not be read (because ENDd is an incorrect spelling) and everything works fine. If you remove the "d" so that it reads "END" and you rescan the file, then you'll see that you cannot play the wav file. Any wave file will do, as long as it is long enough (or make the times shorter).
Created attachment 663 [details] code file which allows to see where the problem is located Attached is Parse.pm which allows to make the problem disappear and reappear. The bug is caused by a missing value for $attributesHash->{'SECS'} which causes a test in the "processAnchor" routine to fail. I tried to locate the source/cause for the missing value. You can find the changes I made by looking for comments market with ## (instead of a single #). They are at line 305 and following. I have made two changes to the code: These lines were moved out of the conditional execution path "if (!$lastpos)" into the general execution path: $::d_parse && Slim::Utils::Misc::msg("Reading tags to get ending time of $filename\n"); my $ds = Slim::Music::Info::getCurrentDataStore(); my $track = $ds->updateOrCreate({ 'url' => $filename, 'readTags' => 1, }); As this didn't resolve the problem I added a log line to see whether the previous code did or did not determine the value of "$track->secs()" which causes the failure of the "processAnchor" routine: $::d_parse && Slim::Utils::Misc::msg("parse: wb1: SECS: " . $track->secs() .. "\n"); I noticed that when this line is executed, then the problem disappears because "$track->secs()" gets executed. When this line is commented out the problem reappears. I do not know what this command does.
Wolfgang, any lines that start with $::d_ are debug messages. In this base, its a message that is sent to the log when d_parse debugging is enabled. Usually, they should not cause this kind of interracting, but occasionally they do execute function calls and different effects occur when debugging is turned on. This, too, should probably be fixed. If turning a debugging switch on can potentially fix bugs, it can make for a tough time debugging :)
Michael - have you had a chance to look at this?
only briefly. I've put together a test file based on the attached cuesheet and reproduced this bug (as of revision 3962). I haven't worked out a fix yet. I'll be out computer range for the next two weeks, but if someone hasn't squashed this by early september, I should be able to come up with something fairly quickly.
Michael, any status on this fix?
If the SECS attribute is missing, that implies some problem with the audio file associated with the cue sheet. Michael, are you going to look at this?
Fixed in subversion change 4609
This bug still exists with SlimServer Version: 6.2.1 - 5194 - Windows XP - EN - cp1252 although it was supposed to be fixed. In particular, a .CUE file with a last line reading � REM END 51:38:15� is captured properly into the SlimServer�s database (i.e., all titles of the cue file a properly inserted into the database with correct timing information), but when I try to play any title of the cue file, I get a message indicating �CAN�T OPEN FILE FOR:�. The parser properly interprets all REM END statements in the CUE file, including the last one. This can be seen by setting the time such that it is shorter than the total length of the associated WAV file. When there is no REM END statement in the last song, then the WAV file plays fine. The REM END statement causing trouble does not have to be the last line in the CUE file; i.e., it can be followed by other statement such as "GENRE".
Can you please supply the CUE sheet that is giving you trouble? The one attached so far here works fine (save for the corruption of 'END' into 'ENDd')
Created attachment 1027 [details] patch (for 6.5 at least) A recent change has DBIStore using 'CONTENT_TYPE', but the parse module is still using CT to fill attributes. This means the files in cue sheets are given nothing for a content type and fail to show up in browseDB since it does a find for audio types only.
basing on a sample I have, looks like duration is ok, but size is 0. that's why it fails to play. the parser doesn't seem to take care of audio_size.
Hello KDF, I did a few checks and here are the results: 1: The 6.2.1 code with the 1st attachment above (cue file) causes the same problem as the one I reported in comment #13; this is with the last line corrected from "ENDd" to "END". 2: The patch changing 'CT' to 'CONTENT_TYPE' does not work on my 6.2.1 installation, but causes another strange behavior. 3: I don't understand where you see size = 0 (your comment #16). Is this in the database? When I list the tracks in the browser, they all have the correct lengths. Please advise which additional info I can provide? I can do logs, but didn't find any that gave me a clue on what is going on. I created a new attachment (logfile1.txt) which logs d_files and d_parse. To create the logs I recreated the database from scratch (there are only two cue files, one of which is the one from 1: above). Then I hit play for the song "Loving is easy". logfile2.txt is the same, except now the last END statement is removed and the song plays normally. I hope all this helps. W.
Created attachment 1028 [details] logfile1.txt - logfile with problem
Created attachment 1029 [details] logfile2.txt - logfile without problem
Wolfgang, you'll have to ignore much of my comments. i was working with 6.5 (as I do). It is very different from 6.2.1 (hence CONTENT_TYPE). I am getting close but as yet unplayable. Once I have that, I'll see how possible it is to merge into 6.2.2 thanks for the log output.
ok, I have the attached sample working in 6.5 (will test more, clean and check in later). What I have noticed is that this will not work in the case of WAV or MP3 playback. Currently only FLAC allows a single file to be split. WAV->FLAC and FLAC->FLAC is ok. The sample case above is a WAV basefile, so it only plays the full file if the player is set to bitrate limited (MP3 output). Clearly, this requires some sort of decision on the limit of support. Having to convert WAV->FLAC->MP3 for this case might get insane (I certainly dont want to have to write it) my files are a big mess right now, so once I get a clean diff, I can see what this is going to take for 6.2.
This sample should now work with 6.5b1 as of change 5253
Created attachment 1031 [details] patch for 6.2 (ugly) This patch does seem to do the trick. What is ugly is that it is a complete force. For some reason, asking for $track->size suddenly makes the track object contain the size and duration information. without this line, the object has only a subset of the attributes and the indexed tracks can't get a size. I don't understand this aspect of the code. what causes an object to spontaneously expand? dan? what is the right thing to do in order to get the full track object?
KDF, Thanks. I tested your patch for 6.2 and it works for me (so far only very little testing). I'll run my system with this for a while and let you know whether how reliable this turns out to be.
As promised in my previous comment, this is just to confirm that after having SlimServer run continuously for almost 2 days, KDF's patch does the job.
Dan: can you comment on this? Can we fix for 6.2.2?
Both patches look good to me.
ok, committed to 6.2.2 at change 5277 for Nov 22 nightly builds.
The fix in the 6.5 (r5253) branch breaks the parsing for flacs with embedded cuesheets. The reason is that the processAnchor function in Parse.pm is also called by the getTag function Flac.pm. In this situation, $attributesHash->{'AUDIO_SIZE'} is undefined (but $attributesHash->{'SIZE'} is properly defined). The result is that $attributesHash->{'AUDIO_SIZE'} is set to zero. In _preCheckAttributes in DBIStore.pm the SIZE value from Flac.pm is assigned to the key audio_size (note lowercase). The result is that there is a key AUDIO_SIZE (uppercase) which is zero and a key audio_size (lowercase) with size equal to the size of the whole album. In the while loop in the newTrack function in DBIStore.pm, the audio_size key in columnValueHash is set to either zero or the album size depending on the position of 'AUDIO_SIZE' relative to the position of 'audio_size' in the attributeHash. The result is that some tracks gets audio_size equal to zero in the database. These tracks become unplayable. I have reverted change 5253 in the 6.5 branch, and instead applied the patch commited to the 6.2 branch (r5277) With the 6.2 patch applied to 6.5, both flacs with embedded cuesheet and a wav with the cuesheet attached to this bug works correctly in 6.5. I therefore suggest that you revert change 5253 and instead apply the 6.2 patch to the 6.5 branch.
I seem unable to reopen the bug. Am I overlooking something, or has the Bugzilla behavior changed?
KDF has previously stated that his patch is ugly. I am not sure if it really is that ugly Looking at the below code (cut from Parse.pm), the patch consists of adding the first line: $track->{'SIZE'} = $basetrack->audio_size; # our tracks won't be visible if we don't include some data from the base file for my $attribute (keys %$basetrack) { next if $attribute eq 'id'; next if $attribute eq 'url'; next if $attribute =~ /^_/; next unless exists $basetrack->{$attribute}; $track->{uc $attribute} = $basetrack->{$attribute} unless exists $track->{uc $attribute}; } The reason that extra line is needed is the %tagMapping hash used in _preCheckAttributes in DBIStore.pm. To get correct copy of tags from basetrack to track, an inverse of %tagMapping has to be used at this point in Parse.pm The line $track->{'SIZE'} = $basetrack->audio_size; does that inverse mapping for the 'SIZE' parameter. I don't know if the other parameters in %tagMapping has to be copied from basetrack to track. If so, they have to be treated similarly as the 'SIZE' parameter. I think it is cleaner to do the inverse mapping at this point in the code (where all the other paramters are copied from basetrack to track), instead of special handling the processAnchor function.
steinar, would you be willing to post a diff for your working state? thanks.
something very strange is at work here, since comments on this bug say 6.2 cue sheet support works, yet bug 2752 reports that they are broken. bug 2758 also reports broken cue support for 6.5 external, yet my tests have passed. it seems that we need a good test set of media and a document to track all the custom specs slimserver is now supporting for CUE.
Created attachment 1084 [details] Patch for 6.5 Diff as requested by KDF. With this patch, I have no problems with embedded cue sheets.
Thanks. I've merge into 6.5 at change 5484. Please let us know if this solves all the issues. We can then close this off and handle the rest of the CUE reports elsewhere.
marking as fixed after testing with a few internal cue's that I have made here. please report new issues as new bugs. if involving internal cue sheets, please include information on the method used (was hard to figure out just one format for my tests) and attach a dump from metaflac for testing. thanks