• Status New
  • Percent Complete
  • Task Type Patches
  • Category Playlists
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Jonathan Gordon - 2008-09-16

FS#9407 - bookmarks overhaul

this patch is a WIP to update the bookmark system.

the change is to save “resume points” into the current .playlist_control file and copy that as a .bmark file. Right now it works fine to load and save (And even list the bookmark resume points).

What doesn’t work yet (in bmark.3.diff) is the recent bookmarks menu item.

So far, the only downside I have found is that when you open a .bmark file the current playlist has to be stopped (it gets bookmarked automatically so if you cancel the .bmark it will resume anyway).
I’m hopeing that this isnt a big enough issue to reject the patch.

Jonathan Gordon commented on 2008-09-16 08:46

ok, turns out sscanf() isnt compiled for targets so this patch uses parse_list() instead

Jonathan Gordon commented on 2008-09-16 10:16

ok not much changed but getting this one out early because i changed the .bmark resume position format a bit so it can be displayed a bit better (shows the track time instead of file offset..)

also, autoload from the filebrowser should work like svn (so even if the bookmark only has one point it will still show the listing (unless we want to get rid of that?) )

Magnus Holmgren commented on 2008-09-16 11:00

It took a little while before I realized how to test it; i.e., by creating a bookmark from the WPS context menu (which I never do, in normal usage). After having passing that little hurdle, I noticed the following things missing:

* Restore of playlist seed, shuffle on/off and repeat mode (seed and shuffle are very important, IMO).
* Display resume index (nice, but not as important).
* Display resume time in minutes:seconds (important too, if you have multiple bookmarks for a file).

And looking at the code, it seems the list is limited to 10 bookmarks.

Magnus Holmgren commented on 2008-09-16 11:02

For the record, my comments were for version 4, since version 5 was posted while I was testing version 4. :)

Jonathan Gordon commented on 2008-09-16 11:09

hmm, I thought playlist shuffle/seed was handled automatically.. need to check that.
the other two points are either fixed in 5, or will be in 6 which I’ll upload in a few hours.

and yeah, should have mentinoed that you need to create with the WPS context menu… autocreating isnt done, and recent bmarks also isnt done.

thanks for testing

Jonathan Gordon commented on 2008-09-16 12:34

ok, v6… the list now looks exactly like svn.

I’ve also removed almost all the old code to make it a bit easier to work with. lots of TODO’s if anyone wants to help out.

I’m not sure how I want to implement the recent bookmarks without stealing either lots of ram, or hitting the disk, suggestions welcome.

autocreating bookmarks if its set to “yes”

Jonathan Gordon commented on 2008-09-22 15:26

.6 looks totally broken :p
this version adds talking bmark list.

suggestions on how to do the recent bookmark list???

Magnus Holmgren commented on 2008-09-22 18:16

I’m not sure what the problem is, really. What would you need the ram for?

Jonathan Gordon commented on 2008-09-28 08:24

ok, this has the start of th recent bmark list, also it seems that creating bookmarks from a stopped playlist never actually worked (as in autocreate when playback is stopped)… this fixes it…

Steve Bavin commented on 2008-09-29 09:19

Hi Jonathan,

Been away for the past fortnight so not yet had time to look at any of this, but I’d suggest that (long-term) the recent bookmarks list should be a more-general “recent stuff” list, i.e. not be tied into bookmarks at all. (I personally don’t use bookmarks much, but would like to be able to just see the last n files I selected…).

This might be worth considering at this point, if not entirely OT.

Magnus Holmgren commented on 2008-09-29 17:26

Had a quick look at version 9 diff, and I don’t understand the explanation for bmark_stopped_position. I mean, it wasn’t needed before, so why now? :) Is it because you removed audio_pause() from bookmark_autobookmark() perhaps?

Jonathan Gordon commented on 2008-10-01 09:43

When stop is pressed the screen calls autobookmark() before calling audio_stop(). Now to be able to speak the menu (if its set to ask) we need to actually stop audio, but when audio is actually stopped the id3 struct is memset to 0 so the resume point in the actual track is lost. To get around this we could either add another var to global_status to store the seconds position (which is what bmark_stopped_position stores) so make it global to the bookmark file.

The other option is creating the bookmark in the playback thread but I decided thats a bad idea because pausing there for user input is probably a Bad Thing (TM)

Mark Ganson commented on 2008-11-03 18:07

I use my player (Sansa e260v1) for listening to audiobooks. I often find myself rewinding immediately after resuming a bookmark, especially if it has been a couple days or so since last listening to a book. I was thinking it would be a nice feature for bookmarks if the bookmarked position could be automatically jumped back 5 or 10 seconds. That way, when you resume listening (usually to an audiobook or podcast) you would hear again the last 5 or 10 seconds prior to the bookmark, kind of re-acquainting yourself within the context of what you were listening to when you stopped.

The default behavior could be to just resume right where you left off, with the alternative options to backstep 5 seconds or 10 seconds (or perhaps some other figure, but I think 10 seconds seems about right). Probably the way to do it is with a jumpback variable (0, 5, or 10 seconds) which would be subtracted from the current position (in seconds) prior to creation of the bookmark, checking, of course, that we don’t rewind past the beginning of the file. This might be a good place to add in such a feature while bookmarking is getting re-worked.

Magnus Holmgren commented on 2008-11-03 18:28

I don’t think this patch is the right place for auto-rewind. The resume point is a byte offset in the file, where we know there’s data ready to be decoded (allowing a codec to just start buffering at that offset and start playing that data). Therefore, any auto-rewind should to be done by the playback code, either before writing or after reading the bookmark (in other words, the bookmark code isn’t affected by it).

Steve Bavin commented on 2008-11-13 13:58

I agree - the auto-rewind is not bookmark-related. (I don’t think it should be headphone-related either, but that’s another story!)

Jonathan Gordon commented on 2008-12-22 14:00

bump/resync/minor update… I’ve added code to handle deleting bookmarks but nothing actually calls it yet so its untested. I’ve also had a go at making the mrb broswer a bit more readable (possibly needs more work), and also I’ve added the track name to the resume info instead of having to load the playlist to find out the track name, the advantage of this is we dont actually have to stop playback when browsing .bmark files (or the mrb browser), the disadvantage is that we dont have the option later of displaying info from the tracks id3 data (although this might be possible if the playlist control/.bmark file is loaded into the plugin buffer…

There are a few TODO’s still in the patch, but im not sure what else is missing… so test please.


Available keyboard shortcuts


Task Details

Task Editing