Cue sheet support

[sn4kebite]

Hi, these commits adds support for cue sheets. These changes are based on some changes I did last summer but didn't have time to finish.

This works by adding some intrusive changes throughout the code, especially the transcoding and playlist parts. Most importantly it passes a track parameter around to indicate which track in the cue sheet should be played. The cue sheet is then parsed and the correct track is fetched, along with the proper time offset and length. The file pointed to by the cue sheet is then decoded with ffmpeg using these time codes.

The reason I'm going with ffmpeg here is that in theory you can provide pretty much any file format with cue sheets, and while the most common is FLAC, there are also some less popular formats such as TTA and TAK which are often used (TTA is fairly common in my case). This also adds another issue, which is missing audiolength for the last track when the format is unknown to cherrymusic; the first tracks uses the start time of the next track as their end time. I guess this could be solved with some ffmpeg magic, unless it's preferable to implement this natively in TinyTag (which I've so far only had a brief look at).

As far as I can see normal playback and transcoding is not affected (works for me), and all unit tests pass (yay), so there shouldn't be any issues for regular usage. Also I'm open for suggestions for any changes or other feedback.

Thanks, Jon

[coveralls]

Coverage decreased (-1.73%) when pulling ab51037e3cc728566919fdbd1cfb124e2e2c5bf2 on sn4kebite:cuesheet into 8b4ad25e2ea59a0468ae8292e8cb5ec25391b666 on devsnd:devel.

[devsnd]

Wow, that's really cool! You've worked through the full stack of CM to create that feature, very impressive indeed! :)

I'll look through it on the weekend so we can get it merged soon :icecream:

[devsnd]

Hi @sn4kebite, I'm sorry, I didn't get to review your code yet! I'm quite busy at the moment, but I hope that I'll get to it soon :)

[sn4kebite]

Hi, thanks for the response. If we can solve other issues by using another approach, I guess that's the way to go. However I didn't quite get what you mean; by duration, do you mean track length, or something like an time interval? Passing the start time should also work, as the cue sheet code could easily figure out which track the given time position belongs to (the last track that starts before or at the given time). I saw that there's already some references to starttime in the code, but I haven't looked too much into that.

[devsnd]

Sorry, I didn't make myself too clear. What I mean is exactly what you mentioned: Using the starttime and a new attribute duration for creating the tracks that make up the cuesheet. Right now you are parsing the cuesheet inside audiotranscode, but audiotranscode actually does not need to know any detail about cuesheets.

I though it could be handled like so:

So audiotranscode does not need to know anything about cuesheets... well actually nobody knows about cuesheets but the file-listing. Everything after that can be handled using start and end time of a track.

[sn4kebite]

Yeah, that sounds like what I was thinking. I've been playing around a bit with the code, and the only issue I see so far is that when fetching metainfo, we also need to know which track in the cue sheet to fetch metainfo for. This could be solved by passing starttime to getsonginfo, but I guess that's not the ideal way to handle that (I'm passing track currently).

Edit: Also, we still need to pass the filepath for the cue sheet instead of the audio file itself. The files does not necessarily have the same base name (you can have eg. foo.cue and bar.flac), so you need to know the filepath to the cue sheet; the audio filepath can easily be resolved by parsing the cue sheet. I currently added this parsing back into decode, but I guess we could also put it in the trans handler or whatever seems more fitting.

[sn4kebite]

Okay, so I basically just replaced track with starttime and duration. I'm currently passing starttime to getsonginfo as I don't see any other way to do that other than reintroducing track. I left a list with issues and other notes in the commit message .

[devsnd]

Yeah you're right, there is something I need to do before this can be solved elegantly: Allowing to pass meta-data directly into the filebrowser, instead of fetching it after the fact asynchronously. I didn't think of that.

If you look inside res/templates/mediabrowser-file.html you can see that there is a unloaded css-class for the file. This css-class is the reason for the CM client to load the meta-info for that track. When listing a cuesheet, we already know all the information up front. There is no need to fetch the meta-data afterwards. We can simply render it directly into the browser and leave out that unloaded class.

I've opened another issue for the one thing that's left to do to make this work: after we show the correct meta-data in the file browser, they have to be passed to the playlist. Essentially that means not to call getmetainfo twice (once for the filebrowser and once for the playlist) but instead transferring all meta-data from the filebrowser to the playlist if it is already available.

[coveralls]

Coverage decreased (-1.72%) when pulling 73f97cd16c43c521a8c98b6abe71d33417bb6815 on sn4kebite:cuesheet into c305c9ad74ef1f94c368b76bbd65bdabefb562fd on devsnd:devel.

[sn4kebite]

So I never got around to finish this because of summer and stuff, but finally found the time to do so. The last commit lets listdir pass metainfo directly without the need for calling getsonginfo. The metainfo is now also passed to the playlist manager, so getsonginfo is never called for cuesheets (or potentially other media which provides metainfo when listed), and only once for single tracks (which I guess should solve #460). As a bonus the cuesheet is now parsed only once per file instead of once per track.

[coveralls]

Coverage decreased (-1.72%) when pulling 7b9eb0149f69050fc819faf347d0fbbfa76e7d9d on sn4kebite:cuesheet into c305c9ad74ef1f94c368b76bbd65bdabefb562fd on devsnd:devel.

[coveralls]

Coverage decreased (-1.75%) when pulling 1b2354d3a0aa1bfa9f0d7861e52f547abc9da84d on sn4kebite:cuesheet into c305c9ad74ef1f94c368b76bbd65bdabefb562fd on devsnd:devel.

[devsnd]

Great, I'll try to look into it this evening :+1:

[coveralls]

Coverage decreased (-1.72%) when pulling c8efb83e79d0cffb3c6ed3c6039322f69c99a029 on sn4kebite:cuesheet into c305c9ad74ef1f94c368b76bbd65bdabefb562fd on devsnd:devel.

[devsnd]

Hi Jon, I'm sorry it toomk me so long to take a look on your code, but there are still multiple issues with it, and I didn't find enough the time for a review until lately. So let's break it up in parts:

audiotranscode

You a added a new type inside audiotranscode to handle the cue sheets, but audiotranscode should only take care of transcoding audio nothing else, just as you've noticed yourself. Additionally the cuesheet may contain all kinds of data not even supported by ffmpeg and therefore needs be handled in a different way. More on that later.

I however recognise the problem of the DURATION flag for ffmpeg. audiotranscode should be able to hadle optional flags. This however is not so urgent as you have set the duration to 100000 seconds or ~27 hours, so I guess we can live with the workaround for now.

cuesheet

Your cuesheet module has some flaws: Your Track has a member nextstart, which is only set by an external call in the metainfo.py. This makes the code very hard to maintain. It would make a lot more sense if the tracks would have a duration, which is set when first parsing the cuesheet. I understand that you cannot know the duration of the last track, but then this should not matter anyways (as long as the last track is less than ~27 hours long)

I like that you made sure that a cuesheet starting with a utf-8 BOM would be parsed correctly, but some comments would have helped to understand what's going on there.

I wanted to use your module to try out some things myself, but I didn't understand the hierarchy of CDText, Descriptors and Tracks. I'd love to see some comments on top of the module to see how it can be used, how to iterate over a set of tracks and their meta-data etc.

cherrymodel

I have to admit that I don't know how the cuesheet format is specified, but to me it looks like your usage of the cuesheet model would only support cuesheets that have a single track in it. But cuesheets may contain multiple files AFAIK.

You are making the cuesheet to a very special case, while in reality it is not. It's just a playlist. It would be a lot more future proof, if you could just add a handler that takes care of all the files ending in 'cue' which then return a MusicEntry that treats playlists like folders. This would allow for adding 'pls' or 'm3u' handlers later on, which would then make playlist imports easy.

metainfo

The metainfo.py was put in place, because we changed the ways to get meta-information for tracks to often, so it's merely a class for abstraction of different library calls and allowing unified access to the data. You have however put some of the essential logic of your cuesheet module inside it. I understand you wanted to put code that does similar things in similar places, but most of it should remain in your module or in the addCueSheet call

The general Problem

To quote what I've said before

So audiotranscode does not need to know anything about cuesheets... well actually nobody knows about cuesheets but the file-listing. Everything after that can be handled using start and end time of a track.

(emphasis added)

In fact, I'd like to handle cuesheets and playlists (pls, m3u) in the same way folders are handled. You could then browse a cuesheet and select a single track, or add all tracks without accidentally selecting any other tracks from the same folder. I'm sorry if I haven't made that clear, but this is the only elegant way to handle this problem.

---

I hope you understand all of my critique. I like to see when people like CM and I love it when people want to improve it. But I cannot accept patches that will make it harder to maintain the program, since we are already knee-deep in code-dept and we need to learn from our mistakes. Also I'd like to apologise again for not responding any quicker after you having made this much effort. In any case, I'd still like this feature to arrive in CM!

[devsnd]

I was just looking around and found this:

http://digitalx.org/cue-sheet/examples/#example01

It might make for a good test-suite...

[devsnd]

Hey Jon, I have created new feature branch which contains a rough first version of cuesheet integration as I'd implement it.

https://github.com/devsnd/cherrymusic/tree/feature_cuesheet

I have completely rewritten the cuesheet parser, I hope you are not mad... :facepunch: But this one now is really easy to use, supports multiple files per sheet and is completely self-contained.

cue = CueSheet(filename)
for track in cue.tracks:
    print('track %s starts at %s and is %s seconds long' % (
        track.filename, track.start_time_abs_sec, track.duration))

I haven't changed anything in the frontend yet: everything works now by tricking the frontend into thinking the cuesheet is a folder and listing the files inside, which works really well.

Next thing would be to integrate the changes you already made for the meta-info to be preloaded and not loaded again by the MediaBrowser, so that the titles, duration and so on are being shown correctly in the file browser. I seems you have already implemented everything needed there and I only have to cherrypick! :+1:

I thought, instead of going through the transcoder first, we could instead make the frontend (the jplayer) do the work for us (skip x seconds, start next song after y seconds of playback) and care for the transcoding later.

Maybe we can work together to finish this? :dancer: :icecream:

[sn4kebite]

Hi, sorry for the late reply.

Treating the cuesheet like a directory actually sounds like a great idea. Also the cuesheet.py I added was taken from another small side-project I was working on some time ago, and badly needed some cleanup anyway.

By having the frontend do the work, do you mean just delivering the file without caring for position in the transcoder (as in, deliver the entire file)?

The cue type in the transcoder was added to deal with the track parameter and exotic filetypes. We can easily get rid of this type now that it uses starttime and duration by asking for the audiofile instead of the cue sheet, however any filetypes that aren't specifically supported won't work, and some decoders like the flac decoder needs some special attention to its arguments to work correctly (seems to need --skip +duration without hours specified to work for me). The last point here is probably moot if I understood you right in not handling times in the transcoder though.

That aside I agree with the issues. And like I said in my initial post, I'm open for any changes.

[tilboerner]

Great job getting the ball rolling, looks like cuesheet support is almost ready.

Treating cuesheets like directories makes the most sense, I think, because we get to keep the interface simple. It also offers the chance of keeping the cuesheet logic very localized. What's the reason for letting the client handle start and end time offsets? I'm forgetting if seeking is a problem in the transcoder. Because if it's not, and if start and duration can become transcoder arguments, the client wouldn't need to care the slightest bit about where the audio is coming from.

I agree it would be good to have some tests in place, as well as some precautions for unreadable/malformed .cue files.

[devsnd]

I'm forgetting if seeking is a problem in the transcoder. Because if it's not, and if start and duration can become transcoder arguments, the client wouldn't need to care the slightest bit about where the audio is coming from.

Essentially that's the problem right now; Because we support that many different programs to transcode and the syntax varies for all of them, it takes some time to make sure everything works as expected. It's somewhat easily solvable, but only adds problems to the issue at hand, i'd say.

By having the frontend do the work, do you mean just delivering the file without caring for position in the transcoder (as in, deliver the entire file)?

Essentially yes. I thought going for the frontend first might be a good idea to get everything running; The server supports paritial HTTP requests and most sane file formats have a good way to handle seeking (i'm looking at you mp3!), so seeking on the client-side isn't such a great overhead. It's possible to instruct the jPlayer to just seek to any part of the stream, so lets use that.

So essentially we're delivering the whole file, but jPlayer and HTTP 206 (and browser caching) take care of the rest for us for all natively supported file types. We can later than make transcoding work correctly.

The issue with the transcoding and seeking within transcoded files then essentially boils down to issue #390 and #275.