Support for multiple FILE commands in CUE. #434

oleg-kuh · 2020-10-08T22:00:31Z

The idea is to set last seen filename for current track.

mherger · 2020-10-12T07:05:57Z

Thanks for this patch!

I see that you removed most of the $inAlbum checks @marco introduced a few years back. Please see the comment block around line 127. Are you sure you aren't breaking any of that logic?

oleg-kuh · 2020-10-12T18:53:29Z

Hi Michael,

I removed $inAlbum checks only from FILE processing block. I suppose it was used here to make sure (and enforce) that FILE command comes before list of tracks. But in fact there could be multiple FILE commands and FILE can appear anywhere in a file (like in a middle of a list of tracks) so this check is not correct.
I tested this patch on collection of CD and Vinyl images (with single and multiple FILE commands) and didn't notice any regressions.

michaelherger · 2020-10-17T06:25:54Z

Will you be around to help out if this change turns out to cause problems to other users? I'm not using CUE sheets myself. And I hesitate to merge a change to (mostly) working functionality, which provides a feature nobody has been asking for in years...

oleg-kuh · 2020-10-17T17:51:08Z

Yes, I can support it.

michaelherger · 2020-10-20T20:57:35Z

Ok, could you please make sure all the comment around line 126 still makes sense after your changes? We can then give it a try.

oleg-kuh · 2020-11-02T01:31:01Z

Comment in line 126 actually says nothing about how FILE command is processed. Added small explanation.

mherger · 2020-11-02T20:51:58Z

Ok, let's give this a try. Thanks!

StuartRob · 2020-11-07T18:08:58Z

I'm just a user but I think that this change might have broken something. I have Logitech Media Server Version: 8.0.0 - 1604555568 @ Thu Nov 5 2020 and scan now fails on .cue files which previous versions of LMS have been perfectly happy to scan.

michaelherger · 2020-11-07T19:14:34Z

@StuartRob would you mind sharing your scanner.log file after a scan crashed?

michaelherger · 2020-11-07T19:19:12Z

Sample cue sheet from the forum thread:

TITLE "ensemble pearl"
PERFORMER "ensemble pearl"
REM DATE "2013"
REM DISCNUMBER 1
REM TOTALDISCS 1
REM DISCID 420E1E06
FILE "01 - ghost parade.wav" WAVE
TRACK 01 AUDIO
TITLE "ghost parade"
ISRC US58L1354401
INDEX 01 00:00:00
FILE "02 - painting on a corpse.wav" WAVE
TRACK 02 AUDIO
TITLE "painting on a corpse"
ISRC US58L1354402
INDEX 01 00:00:00
FILE "03 - wray.wav" WAVE
TRACK 03 AUDIO
TITLE "wray"
ISRC US58L1354403
INDEX 01 00:00:00
FILE "04 - island epiphany.wav" WAVE
TRACK 04 AUDIO
TITLE "island epiphany"
ISRC US58L1354404
INDEX 01 00:00:00
FILE "05 - giant.wav" WAVE
TRACK 05 AUDIO
TITLE "giant"
ISRC US58L1354405
INDEX 01 00:00:00
FILE "06 - sexy angle.wav" WAVE
TRACK 06 AUDIO
TITLE "sexy angle"
ISRC US58L1354406
INDEX 01 00:00:00

michaelherger · 2020-11-09T05:15:23Z

@oleg-kuh there's something wrong with this change. We're seeing crashes in the CUE sheet handling. Please see the forum thread linked above. There are two scanner.log files which don't show any obvious failure, but just end at some point.

I'll see whether I can find something. Otherwise I'll have to revert this change.

mw9 · 2020-11-09T15:02:47Z

I have uploaded a sample flac file with embedded cue sheet to the forum. This does trigger a problem. It may help diagnose.

See this forum thread post:
https://forums.slimdevices.com/showthread.php?113194-Scanning-problem-after-a-recent-8-0-0-update&p=993877&viewfull=1#post993877

michaelherger · 2020-11-09T19:02:37Z

There indeed seems to be an infinite loop somewhere: I pointed the scanner at a folder with less than a dozen files with CUE sheets. After a short time of no visible activity the scanner used 1.5GB of memory. I guess at some point it crashes after running out of memory. I'll investigate.

michaelherger · 2020-11-09T19:12:32Z

Slim/Formats/Playlists/CUE.pm

-	my $lastpos = $tracks->{$currtrack}->{'END'};
-
-	# If we can't get $lastpos from the cuesheet, try and read it from the original file.
-	if (!$lastpos && $filename) {


@oleg-kuh This is where we enter an infinite loop. We must not enter this condition when we're done with a file, or it'll go back to Slim::Formats->readTags() which in turn will call Slim::Formats::FLAC->getTag(), which will call the CUE sheet parser again. Here's a snippet from a backtrace:

[20-11-09 20:07:17.2417] Slim::Formats::Playlists::CUE::parse (103) Backtrace: frame 0: Slim::Utils::Log::logBacktrace (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 103) frame 1: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 2: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 3: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 4: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 5: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 6: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 7: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 8: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 9: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 10: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 11: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 12: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 13: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 14: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 15: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 16: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 17: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 18: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 19: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 20: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 21: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 22: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 23: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 24: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 25: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 26: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 27: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 28: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 29: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 30: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 31: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 32: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 33: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 34: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 35: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181) frame 36: Slim::Formats::readTags (/Users/mh/SynologyDrive/git/server/Slim/Formats/Playlists/CUE.pm line 463) frame 37: Slim::Formats::Playlists::CUE::parse (/Users/mh/SynologyDrive/git/server/Slim/Formats/FLAC.pm line 124) frame 38: Slim::Formats::FLAC::getTag (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 191) frame 39: (eval) (/Users/mh/SynologyDrive/git/server/Slim/Formats.pm line 181)

After only a few seconds there already were thousands of recursions...

Re-introducing this check would "fix" the recursion for me. @oleg-kuh - any good reason why you removed it?

mherger · 2020-11-10T05:11:28Z

@oleg-kuh - I've reverted this change for now. I'd like to stabilize LMS 8.0 to be able to release it. Please look into the issues reported here and submit a new PR. I'll be happy to merge to an upcoming 8.1 and give it another try. Thanks for your understanding.

oleg-kuh · 2020-11-10T05:17:48Z

Pull request #452 should fix the issue.

michaelherger · 2020-11-10T05:42:55Z

Thanks @oleg-kuh! I re-applied both changes. Please everybody give it some good testing.

bpa-code · 2020-12-07T19:27:17Z

This chnage has broken some other users CUE file. They worked from 7.9.2 and broken on 8 Oct in LMS 8.0
See https://forums.slimdevices.com/showthread.php?113355-LMS-8-0-0-running-but-does-not-play-files&p=998249&viewfull=1#post998249

mherger · 2020-12-07T20:28:31Z

Thanks @bpa-code for looking into this. Did you manage to create a "disk" to reproduce this issue?

michaelherger · 2020-12-08T12:33:53Z

@oleg-kuh could you please look into this?

This chnage has broken some other users CUE file. They worked from 7.9.2 and broken on 8 Oct in LMS 8.0
See https://forums.slimdevices.com/showthread.php?113355-LMS-8-0-0-running-but-does-not-play-files&p=998249&viewfull=1#post998249

okukharchuk · 2020-12-09T08:03:41Z

Cue sheet in question looks like "non-compliant" to me. I guess such cues supported only by EAC. Supporting them will require significant redesign of existing CUE parser. I think best way for now will be to ignore them. It will require some detecting logic. I can take closer look on weekend.

mherger · 2020-12-09T08:05:50Z

While they are "non-compliant", they did work before this pull request was merged.

oleg-kuh · 2020-12-09T08:11:33Z

I believe before this pull request we were ignoring all the CUEs with multiple FILE commands. So CUE was not used at all.

michaelherger · 2020-12-09T08:17:39Z

I'm not too familiar with the full story. But it's been proven that some CUE sheets which did work before now no longer work. So this is a regression, not a buggy new feature.

oleg-kuh · 2020-12-09T08:24:57Z

Well, the full story is short. Before this pull request all the CUEs that have more then one FILE command were ignored by parser (including non-compliant CUE in question).
I'm sure that non-compliant CUE will not work with existing parser. So I'm proposing add detection logic and skip them just like it was before.
If you have any examples of "compliant" CUEs that do not work after this change I'll be happy to take a look.

michaelherger · 2020-12-09T09:27:15Z

As I mentioned there are cases which worked before and are broken now. I don't know whether @bpa-code can share his with you.

michaelherger · 2020-12-09T14:30:25Z

@oleg-kuh I actually might have been wrong: the test case I have here would indeed previously scan correctly because the CUE sheet was rejected. A solution for the user might be to simply get rid of these useless CUE sheets.

Or could there be an easy way to discover "invalid" CUE sheets and skip these?

bpa-code · 2020-12-09T16:19:52Z

I don't have a vested interest in the CUE file - I just chased down the bug in case it was related to Flac and "-skip" but there are users of these CUE files.
These CUE file "worked" before the 8-Oct change.
These CUE files are generated by EAC which is a widely used application. Users of the CUE files was generated by ver 0.99 and my test one was generated by 1.6. So it has been in used for many years.
It is documented in a Hydrogenaudio Wiki so if it is buggy - why are people not saying so.
User was happy with pre 8-Oct LMS functionality and if ignoring the CUE file is the pre 8-Oct behaviour then why not do that.

oleg-kuh · 2020-12-10T07:28:11Z

See #474

vco1 · 2021-01-15T14:17:07Z

There is (yet) another scenario that's not fully working in 8.1.1: multiple files with multiple tracks each. When scanning these, the files show up in the Album list with file type 'cur'. These tracks can't be played. And shouldn't be visible in the tracklist.
Below an example of such a cue sheet.

REM DATE 2019
PERFORMER "Artist Name"
TITLE "Album Title"
FILE "Artist Name - Album Title 1.flac" WAVE
  TRACK 01 AUDIO
    TITLE "Title 1"
    PERFORMER "Artist Name"
    INDEX 01 00:00:00
  TRACK 02 AUDIO
    TITLE "Title 2"
    PERFORMER "Artist Name"
    INDEX 01 09:2:00
  TRACK 03 AUDIO
    TITLE "Title 3"
    PERFORMER "Artist Name"
    INDEX 01 17:00:00
  TRACK 04 AUDIO
    TITLE "Title 4"
    PERFORMER "Artist Name"
    INDEX 01 26:19:54
  TRACK 05 AUDIO
    TITLE "Titel 5"
    PERFORMER "Artist Name"
    INDEX 01 35:32:00
  TRACK 06 AUDIO
    TITLE "Title 6"
    PERFORMER "Artist Name"
    INDEX 01 42:36:45
FILE "Artist Name - Album Title 2.flac" WAVE
  TRACK 07 AUDIO
    TITLE "Title 7"
    PERFORMER "Artist Name"
    INDEX 01 00:00:00
  TRACK 08 AUDIO
    TITLE "Title 8"
    PERFORMER "Artist Name"
    INDEX 01 07:46:00
  TRACK 09 AUDIO
    TITLE "Title 9"
    PERFORMER "Artist Name"
    INDEX 01 17:00:00
  TRACK 10 AUDIO
    TITLE "Title 10"
    PERFORMER "Artist Name"
    INDEX 01 22:02:00
  TRACK 11 AUDIO
    TITLE "Title 11"
    PERFORMER "Artist Name"
    INDEX 01 26:31:54
  TRACK 12 AUDIO
    TITLE "Title 12"
    PERFORMER "Artist Name"
    INDEX 01 33:50:54
  TRACK 13 AUDIO
    TITLE "Title 13"
    PERFORMER "Artist Name"
    INDEX 01 37:56:26
  TRACK 14 AUDIO
    TITLE "Title 14"
    PERFORMER "Artist Name"
    INDEX 01 43:43:10
  TRACK 15 AUDIO
    TITLE "Title 15"
    PERFORMER "Artist Name"
    INDEX 01 50:45:02

Support for multiple FILE commands in CUE.

f2f7c57

Comment multiple FILE command support.

3ba311b

oleg-kuh force-pushed the public/8.0 branch from c326e26 to 3ba311b Compare November 2, 2020 01:28

mherger merged commit 13b84ce into LMS-Community:public/8.0 Nov 2, 2020

michaelherger reviewed Nov 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple FILE commands in CUE. #434

Support for multiple FILE commands in CUE. #434

oleg-kuh commented Oct 8, 2020

mherger commented Oct 12, 2020

oleg-kuh commented Oct 12, 2020

michaelherger commented Oct 17, 2020 •

edited

oleg-kuh commented Oct 17, 2020

michaelherger commented Oct 20, 2020

oleg-kuh commented Nov 2, 2020

mherger commented Nov 2, 2020

StuartRob commented Nov 7, 2020

michaelherger commented Nov 7, 2020

michaelherger commented Nov 7, 2020

michaelherger commented Nov 9, 2020

mw9 commented Nov 9, 2020 •

edited

michaelherger commented Nov 9, 2020

michaelherger Nov 9, 2020

michaelherger Nov 9, 2020

mherger commented Nov 10, 2020

oleg-kuh commented Nov 10, 2020

michaelherger commented Nov 10, 2020

bpa-code commented Dec 7, 2020

mherger commented Dec 7, 2020

michaelherger commented Dec 8, 2020

okukharchuk commented Dec 9, 2020

mherger commented Dec 9, 2020

oleg-kuh commented Dec 9, 2020

michaelherger commented Dec 9, 2020

oleg-kuh commented Dec 9, 2020

michaelherger commented Dec 9, 2020

michaelherger commented Dec 9, 2020

bpa-code commented Dec 9, 2020

oleg-kuh commented Dec 10, 2020

vco1 commented Jan 15, 2021

Support for multiple FILE commands in CUE. #434

Support for multiple FILE commands in CUE. #434

Conversation

oleg-kuh commented Oct 8, 2020

mherger commented Oct 12, 2020

oleg-kuh commented Oct 12, 2020

michaelherger commented Oct 17, 2020 • edited

oleg-kuh commented Oct 17, 2020

michaelherger commented Oct 20, 2020

oleg-kuh commented Nov 2, 2020

mherger commented Nov 2, 2020

StuartRob commented Nov 7, 2020

michaelherger commented Nov 7, 2020

michaelherger commented Nov 7, 2020

michaelherger commented Nov 9, 2020

mw9 commented Nov 9, 2020 • edited

michaelherger commented Nov 9, 2020

michaelherger Nov 9, 2020

Choose a reason for hiding this comment

michaelherger Nov 9, 2020

Choose a reason for hiding this comment

mherger commented Nov 10, 2020

oleg-kuh commented Nov 10, 2020

michaelherger commented Nov 10, 2020

bpa-code commented Dec 7, 2020

mherger commented Dec 7, 2020

michaelherger commented Dec 8, 2020

okukharchuk commented Dec 9, 2020

mherger commented Dec 9, 2020

oleg-kuh commented Dec 9, 2020

michaelherger commented Dec 9, 2020

oleg-kuh commented Dec 9, 2020

michaelherger commented Dec 9, 2020

michaelherger commented Dec 9, 2020

bpa-code commented Dec 9, 2020

oleg-kuh commented Dec 10, 2020

vco1 commented Jan 15, 2021

michaelherger commented Oct 17, 2020 •

edited

mw9 commented Nov 9, 2020 •

edited