- Status Closed
- Percent Complete
- Task Type Patches
- Category Music playback
- Assigned To No-one
- Operating System All players
- Severity Low
- Priority Very Low
- Reported Version
- Due in Version Undecided
-
Due Date
Undecided
- Votes
- Private
FS#6460 - Cuesheet support
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
cuesheet_core_v3.patch
(23.7 KiB)
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
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”
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. :)
Here is the latest version. For more information, see the wiki page : http://www.rockbox.org/twiki/bin/view/Main/CuesheetSupport
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
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.
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
Yes same occurs here since i use the danram version!
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).
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.
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:
Please give me lots of feedback :)
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
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…
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 ;)
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 :)
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 :)
This one should compile on all targets without any warnings.
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.
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…
Sync to current SVN.
Please report any issues you find, as I’d like to get this committed soon :)
Updated again.
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)
Gary: the warnings are probably a consequence of the failed hunk.
The recent commits are giving me a hard time maintaining this :)
Updated again.