Rockbox

Tasklist

FS#6460 - Cuesheet support

Attached to Project: Rockbox
Opened by Nicolas Pennequin (nicolas_p) - Wednesday, 20 December 2006, 12:53 GMT
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Here is my work on cuesheet support. I think I've now made enough progress to submit it here to get help in testing.
Basically, this patch adds cuesheet support to the core (as opposed to having it in a plugin), using "the amarok way".
For more information, read the following wiki page : http://www.rockbox.org/twiki/bin/view/Main/WebHome?topic=CuesheetSupport
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 14 February 2007, 14:41 GMT
Reason for closing:  Accepted
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 20 December 2006, 17:29 GMT
Version 4 :
- Made the markers be displayed at the right places when the progressbar doesn't use the full screen width
- Made the tracknumber be updated correctly

I've tested this on an H300 sim and on my H320. It seems to work perfectly well :)
It also compiles fine on the ipod video and nano, on the H10 5GB and the iAudio X5 (normal builds).
However, on Archos targets I get a pointer error I don't understand, repeated several times.
example : "cuesheet.c:290: error: dereferencing pointer to incomplete type"
Comment by Michael Sevakis (MikeS) - Wednesday, 20 December 2006, 18:10 GMT
Yes core, core! :) My thoughts for the record (some repetition from IRC). Perhaps too detailed and dry to read. :P

I do think somehow seekpoint support must be added to playlists. Queue sheets really are just a playlist with seekpoints within tracks, no? An m3u file or anything without subindex support can just have one seekpoint (index 0 - the start). If playback in the UI is controlled through playlists only (even if the calls just pass straight through to audio functions), the playlist engine can just tell audio to do a ff or rw to the correct spot. The WPS can be aware of these points by querying the playlist engine about track information and tell the current playlist to go to the current track's subindex, if any. WPS files can also have a tag for index display and no need to fake indexes as tracks. AB repeats should probably be handled there as well with the needed functionality to get pretty much sample accurate cp markers (shouldn't be too problematic for SWCODEC). All playlists of all forms, even generated should use a generic internal represenatation that covers the features of the various types supported. I'm not yet aware to the extent that's already the case but will be compelled to figure it out!

What would be cool is to have automatic association of queuesheets with tracks in the directory. Good for those classical albums (and Rush albums) if you like tracks organized just like the CD. Say, a .cue matching the file name track.mp3.cue. Include the extension to disambiguate track.flac and track.mp3. Perhaps an album.cue or the like that contains the info for the tracks in a directory which is read automatically. I think I'm getting ahead of things here but those additional feature wouldn't have touch upon the playback engine with a good clean functional break between the audio system and more the user oriented code. I guess I assigned the task to myself in that regard. :)
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 03 January 2007, 21:20 GMT
Here is the latest version. For more information, see the wiki page : http://www.rockbox.org/twiki/bin/view/Main/CuesheetSupport
Comment by Norbert Preining (norbusan) - Thursday, 04 January 2007, 00:58 GMT
Hi Nicolas!
I have included your latest patch into my build, and there is also the progressbar_ycoord patch. Now what I get is that the cue sheet jumping works quit e well, but where the text of author/title etc should be, or only the filename, there is now a list of dotted boxes. Do you have an idea what this might come from? AFAIR this didn't happen when I used cuesheet_core_v4.patch.

Bye Norbert
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 04 January 2007, 03:52 GMT
Norbert: I've never had this sort of bug... Does it happen with all your cuesheets ?
Also maybe check what I wrote about this patch not applying cleanly on the pb_y_coord patch (in the tracker entry about it), fix that problem and see if you still get dotted boxes instead of the info.
Comment by Norbert Preining (norbusan) - Thursday, 04 January 2007, 11:05 GMT
Hi Nicolas!
Well, I have only one cuesheet, I only included it because someone asked for it to be included. Might be a good idea, but I don't use it.
The same happens with a build where I fixed the drawmarkers invocation (see other FS entry, this works now very well!).

Maybe I try do get some other cuesheets ... or can I do something else?

Bye and thanks a lot

Norbert
Comment by Sacha (Angyman) - Thursday, 11 January 2007, 21:40 GMT
Yes same occurs here since i use the danram version!
Comment by Adam (voltagex) - Tuesday, 16 January 2007, 07:09 GMT
This patch may need a little update...
patching file apps/SOURCES
patching file apps/cuesheet.c
patching file apps/cuesheet.h
patching file apps/main.c
patching file apps/onplay.c
patching file apps/playback.c
patching file apps/settings.c
Hunk #1 succeeded at 706 (offset 1 line).
patching file apps/settings.h
patching file apps/tree.c
patching file apps/gui/gwps-common.c
patching file apps/gui/gwps.c
patching file apps/lang/english.lang
Hunk #1 succeeded at 10459 (offset 3 lines).
patching file firmware/export/id3.h
Hunk #1 succeeded at 206 (offset 1 line).
Comment by Jonathan Gordon (jdgordon) - Wednesday, 17 January 2007, 10:34 GMT
Nico, I synced the patch and had a bit of a play (added the settign which is needed).
but....
the reason it shows the music icon is because you gave it the _MPA attribute, the problem is this is the only attribute that gets added to the playlist, now, if you chane the attribute and try to add it other parts complain.
So, what do you want to do? leave the icon, which means there are 2 files possibly the same (in supported mode the extension isnt shown, so you cant tel the difference.) or do you want to hack it up a bit more to get it working with a different atribute?

I'm not sure if we want to allow it to add .cue files in regular folder play anyway...
anyway, I'll leave that up to you. Ive attached the changes needed to settings_menu.c to add the setting, and english.lang (new strings must be at the end, so the one in the patch is out of date).

lastly, there is an unused variable in gwps-common.c on a regular h300 build.
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 17 January 2007, 20:09 GMT
OK, here's a new version of the patch

CHANGES:
- improved gwps-common.c:update()
- fixed bug in the parser with tracks that start at a time greater than 99 minutes
- fixed dotted squares bug
- added setting to disable cuesheet support (in the playback settings)
- removed the MATCH macro in the parsing function
- display cuesheet performer in tracks that have no performer set
- other minor changes

KNOWN BUGS:
- weird behaviour in curr_cuesheet_skip (see the comment)
- possible crash when skipping to a cuesheet shortly after having added it to the playlist

TODO:
- decide how to handle cuesheets in the browser and in playlists:
* attribute and icon of a cuesheet file ?
* "playing the audio file loads the cuesheet" or "playing the cuesheet loads the audio file" ?
* how to prevent both the audio file and the cuesheet from being added to the playlist ?
* should cuesheets have the same name as the audio file or should the names be decoupled ?

Please give me lots of feedback :)
Comment by Norbert Preining (norbusan) - Friday, 19 January 2007, 09:32 GMT
HI Nic!
Strange thing, might be an interference with other patches, but:
- I cannot add a cue file to a playlist
- playing the respective mp3 file does not show any marks
- context menu of a cue sheet, select cuesheet viewer hangs the player
Any ideas ...

Best wishes
Norbert
Comment by Dave Chapman (linuxstb) - Friday, 19 January 2007, 12:17 GMT
As I mentioned on IRC, I think .cue files should be treated as "external metadata" to an audio file. i.e. you play the audio file, and then Rockbox (if cuesheets are enabled) will look for a cuesheet with the same filename, but the .cue extension when it's reading the metadata.

So the .cue reading would be part of the get_metadata() function. This makes it easy to add support for other kinds of chapters/subtracks - e.g. multi-track SID files, FLACs with embedded cue points, chapters in mp4 files.

The downside to that approach is that Rockbox needs to look for a .cue file every time an audio file is played. This is a small performance hit, but less if dircache is enabled. This check would of course only be done if the global "enable cuesheets" setting is enabled.

But the issue that worries me is that the patch only allows for two cuesheets to be loaded into memory at once. I don't understand how this is dealt with in the patch - what happens if you have three files with cuesheets in the buffer at the same time? Or if this feature is extended to support multi-track SID files, what if you have 32 tracks in memory at once with cues?

This seems to be running into the same issues as album art - it's not going to be possible to implement it cleanly until we can store metadata on the buffer...
Comment by Nicolas Pennequin (nicolas_p) - Friday, 19 January 2007, 22:19 GMT
Norbert:
About the crash with the viewer, I discovered it too when I tested on my H320 (which unfortunately I can't do very often). On the sim it worked fine. I'm pretty sure about the cause though so this shouldn't be too hard to fix.
The other issue is a bit more surprising but I have it too so I should be able to find the problem.
Thanks for the report ;)
Comment by Nicolas Pennequin (nicolas_p) - Tuesday, 23 January 2007, 20:58 GMT
Here's a small update of my patch before I start moving to the "loading the audio file loads the cuesheet" approach.
The viewer shouldn't make players crash anymore.
Please report any problem you see :)
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 24 January 2007, 16:02 GMT
Here is the patch for the other approach.
Now each time an audio file is played, a cuesheet file with the same name is looked for.
It is possible to disable the feature completely with the setting in General Settings > playback.

Please tell me what you think :)
Comment by Nicolas Pennequin (nicolas_p) - Friday, 26 January 2007, 15:52 GMT
This one should compile on all targets without any warnings.
Comment by Henri Chapelle (henriavelabarbe) - Friday, 26 January 2007, 15:59 GMT
Tested with today svn refresh, works after a quick fix in settings_list.c
Your inv patch is what I was looking for, I think you should go this way.


Comment by Nicolas Pennequin (nicolas_p) - Friday, 26 January 2007, 19:37 GMT
What quick fix ? I thought it was perfectly in sync... :/

I'm going this way now :)
It's not the way cuesheets were meant to be used but it's the way everybody uses them now and it also happens to be more intuitive...
Comment by Nicolas Pennequin (nicolas_p) - Sunday, 11 February 2007, 21:01 GMT
Sync to current SVN.
Please report any issues you find, as I'd like to get this committed soon :)
Comment by Nicolas Pennequin (nicolas_p) - Monday, 12 February 2007, 20:13 GMT
Updated again.
Comment by Gary Light (evilg123) - Tuesday, 13 February 2007, 04:42 GMT
seeing as you're keeping a close eye on this here's an error report
I tried patching from recent svn 13 Feb 00:32 (fresh, no other patches applied yet)

$ patch --binary -p0 < cuesheet_inv20070212.patch
patching file apps/cuesheet.c
patching file apps/cuesheet.h
patching file apps/tree.c
patching file apps/lang/english.lang
patching file apps/gui/gwps-common.c
patching file apps/gui/gwps.c
patching file apps/tree.h
patching file apps/settings.h
Hunk #1 succeeded at 683 (offset 2 lines).
patching file apps/menus/playback_menu.c
patching file apps/metadata.c
Hunk #1 FAILED at 31.
Hunk #2 succeeded at 2270 (offset -1 lines).
1 out of 2 hunks FAILED -- saving rejects to file apps/metadata.c.rej
patching file apps/settings_list.c
patching file apps/filetree.c
patching file apps/playback.c
patching file apps/SOURCES
patching file apps/main.c
patching file firmware/export/id3.h

I don't know how serious this is as I'm not too savvy with regard to patching/coding. Still compiles properly!
I can confirm that this patch works on the Ipod 5g, nice work.
(on a side note I am getting a TONNE of warnings while compiling right now :S)
Comment by Nicolas Pennequin (nicolas_p) - Tuesday, 13 February 2007, 09:30 GMT
Gary: the warnings are probably a consequence of the failed hunk.

The recent commits are giving me a hard time maintaining this :)
Updated again.

Loading...