Rockbox

Tasklist

FS#11748 - Automatic multi-resume feature: Remember and restore resume position for each track

Attached to Project: Rockbox
Opened by sideral (sideral) - Thursday, 11 November 2010, 00:22 GMT
Last edited by sideral (sideral) - Tuesday, 01 March 2011, 14:45 GMT
Task Type Patches
Category Music playback
Status Closed
Assigned To sideral (sideral)
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Testers / comments wanted!

This experimental patch adds an automatic multi-resume feature that supports complex listening habits, such as skipping back and forth among multiple half-played tracks. It automatically remembers and restores a resume position for each track without user intervention, without relying on the bookmarking system and without requiring any special navigation; resuming works whichever way a track is found (file browser, database, playlist).

The feature is modeled after the podcast/audiobook resume feature of the Sansa ClipV2 / Clip+, IMHO the sole feature of the original Sansa firmware that was better than what Rockbox has. This implementation is more generic, though, and works for all tracks.

In summary, it works like this: Whenever playback of a track is stopped, the current playback offset is stored in the tag database. (The offset is reset if a track is played all the way to its end.) The offset is restored whenever a track is played back again, except for short tracks in case of an automatic song change.

This patch is experimental as it hard-codes some of the behavior and isn't factored out terribly well. Although I have thoroughly tested the feature, I seem to never have triggered execution of some parts of the patch, especially the changes to tagcache_fill_tags and build_numeric_indices. I am requesting feedback on whether these changes make any sense at all.

Furthermore, this patch relies on a few bug fixes to the tag database (see  FS#11721 ,  FS#11722 , and  FS#11723 ). I am attaching to this report another patch that fixes all three bugs for your convenience.

Caveats:

* Only works if runtime statistics have been enabled. Ideally the resume feature would be independent from runtime statistics; perhaps it should be implemented as a separate consumer of the PLAYBACK_EVENT_TRACK_BUFFER and PLAYBACK_EVENT_TRACK_FINISH events.

* Always resumes tracks if possible (under the condition mentioned in the next caveat). If a resume is unwanted, the user can always press the “skip back” button to play the track from the beginning. This should be configurable (always/never/ask user).

* Hard-coded resume behavior: On automatic track change, the next track will be resumed at its resume position if the track is longer than 20 minutes (assuming it is a partially played podcast/audiobook); otherwise, the track will be started anew (assuming a mid-track playback stop for an album track). This feature and its threshold should be configurable.
This task depends upon

Closed by  sideral (sideral)
Tuesday, 01 March 2011, 14:45 GMT
Reason for closing:  Accepted
Additional comments about closing:  Feature has been committed to trunk
Comment by Jonathan Gordon (jdgordon) - Thursday, 11 November 2010, 09:27 GMT
Why is this linked to the database?
Doesnt this have a pretty hefty penalty on track loads?
The resume mid playlist behaviour sounds very wierd to me.

Perhaps it would be better to fix bookmark system to work better?


The database patches look good, we need your full name to be able to accept them though.
Comment by sideral (sideral) - Thursday, 11 November 2010, 10:32 GMT
Thanks for your comments, jdgordon! Excellent questions…

> Why is this linked to the database?

The database already stores similar information such as the playtime, last-played time, and commitid, so storing the resume information there seems to be a logical extension. Also, it is conceivable that one might want to configure a tagnavi view that filters by resume offset (“Show all resumable tracks”).

Additionally, from a practical standpoint, the database is the storage medium that is the easiest to access from the code and that has all the parsing and I/O infrastructure readily present.

> Doesnt this have a pretty hefty penalty on track loads?

No. This patch just adds one database lookup to a long list of lookups already present in tagtree_buffer_event.

The only new dependency is that decoding cannot start until the resume offset has been retrieved from the database (see the change to audio_finish_load_track; tracks[].taginfo_ready is now turned on after handling the PLAYBACK_EVENT_TRACK_BUFFER event). In practice, I could not observe any noticeable difference in behavior or timing on my Sansa ClipV2.

If needed, we could optimize this a bit by retrieving only the resume offset before setting tracks[].taginfo_ready, and doing the rest of the PLAYBACK_EVENT_TRACK_BUFFER handling (the original tagtree_buffer_event) afterwards (as is done today).

> The resume mid playlist behaviour sounds very wierd to me.

You mean the 20 min threshold for automatic track changes?

This feature allows you to queue up partially played podcasts in a (possibly dynamic) playlist and have them all resumed where they were last stopped.

I found this heuristic to work very well in practice; I suggest you try it for a while to see whether you can appreciate it. Anyway, I do suggest to make the threshold configurable.

> Perhaps it would be better to fix bookmark system to work better?

I have given up on the bookmarking system. I have never seen an implementation of (automatic or manual) bookmarking on any player that worked as well as the feature I am proposing.

I have tried Rockbox's automatic bookmarking feature for a while, but it did not work for me for at least the following reasons

* Bookmarks can only be found through the file browser, and not through the database.
* Separate interface to bookmarks, which is different from the usual way of selecting tracks through the database or the file browser.
* Sometimes, no bookmark was created on power-off.
* Sometimes, the recent-bookmarks list was not updated.
* Bookmarks don't work when files have multiple extensions (my podcast-synchronization software unconditionally adds a file-type suffix for all downloads, and thus often creates files ending in “.mp3.mp3”, for instance).

Also, the automatic bookmarking feature's implementation is much more complex and heavyweight than what I am proposing, as a separate infrastructure is needed for finding, parsing, and storing bookmarks. Additionally, the bookmark files are cluttering up the filesystem.

All of this led me to believe that bookmarks cannot easily be made to work seamlessly and automatically for multi-track resume.
Comment by Jonathan Gordon (jdgordon) - Thursday, 11 November 2010, 10:46 GMT
That is all fine, except totally useless for people who don't use the database. Also I don't see why it is accetable that you cant easily copy the bookmark from one DAP to the next (because the database isnt portable).

My suggestion is still fix the bokmark system to make it work better (which I'm happy to accept a total rewrite to do it). Anyway a simpler version of this is just to write a cuesheet (maybe named differently) for your bookmark
Comment by sideral (sideral) - Thursday, 11 November 2010, 11:21 GMT
Thanks for your comments, jdgordon!

> That is all fine, except totally useless for people who don't use
> the database.

This may be a misunderstanding. My resume feature works independently of whether you use the database or the file browser to start find tracks.

The only precondition is that the database has been initialized, which I find to be an acceptable “limitation.”

(Rockbox has similar limitations elsewhere; for example, the database views get updated for new files only if “load DB to RAM” has been enabled, even if “auto update” has been set.)

> Also I don't see why it is accetable that you cant easily
> copy the bookmark from one DAP to the next (because the database
> isnt portable).

You still can create and copy bookmarks with the existing bookmarking feature. This new feature coexists with, and augments, the existing resume capabilities (mid-track resume after stop / on power-on / from bookmark), and does not replace them.

Even if this wasn't the case, I don't see why this would prevent you from accepting this feature. The major use case supported by this feature is reliable multi-track resume, not bookmark portability.

> My suggestion is still fix the bokmark system to make it work better
> (which I'm happy to accept a total rewrite to do it). Anyway a
> simpler version of this is just to write a cuesheet (maybe named
> differently) for your bookmark

I'm afraid this would be a different task for a different developer. Also, it would be much more complex for little benefit and with additional problems, as outlined in my previous comment.


I am asking you (and your fellow developers) to judge my patch on its merits rather than attempting to find the fly in the ointment. I believe the patch is rather elegant and lightweight (just have a look at it) and adds an often requested feature with minimal overhead. So far, I have not seen a single argument that would convince me that the approach I took is broken.
Comment by Alex Parker (BigBambi) - Thursday, 11 November 2010, 11:32 GMT
I'm not a big fan at all of having to have the database initialised. The database being updated only if load to RAM is used is different, it is all within the same feature. Here we have to have a totally useless feature to me (the database) up and running to use a (what seems to be) unreleated one.

Incidentally, this is all moot if we don't have a real name, could you let us know your name?
Comment by Jonathan Gordon (jdgordon) - Thursday, 11 November 2010, 11:45 GMT
"I am asking you (and your fellow developers) to judge my patch on its merits rather than attempting to find the fly in the ointment. I believe the patch is rather elegant and lightweight (just have a look at it) and adds an often requested feature with minimal overhead. So far, I have not seen a single argument that would convince me that the approach I took is broken"

(for better or worse) it is pretty hard to get new features into rockbox because there are so many people with an opinion. What this does mean though is that when they do go in they are about as good and fully flushed out as can be. So please don't take our comments as offensive, just as comments (you can of course ignore them if you want, and all that really means is finding a dev who agrees with you to help you get it accepted.

My reasoning for trying to push you to making this work with the bookmark system is because (quite frankly) the current bookmark system sucks and can be done better and can add this feature simply.
Comment by sideral (sideral) - Thursday, 11 November 2010, 14:58 GMT
BigBambi writes:
> I'm not a big fan at all of having to have the database
> initialised. The database being updated only if load to RAM is used
> is different, it is all within the same feature. Here we have to
> have a totally useless feature to me (the database) up and running
> to use a (what seems to be) unreleated one.

I understand your concern. You'd have to use the “totally useless” database to use this reliable resume feature. But today the situation is worse: You have to enable to “totally useless” bookmarking feature to get an _unreliable_ resume feature, which is arguably “totally useless” by itself.

You will always have the option of not using the database at all. If you elect not to enable the database, you lose several features: browsing by album/artist/whatnot tags, runtime statistics, and – reliable bookmarking.

This feature cannot make everyone happy. No feature can. But it does make Rockbox a competitive, if not the best, firmware for listening to podcasts and audiobooks, which is an important use case. Maybe not for you, but for others like me.

> Incidentally, this is all moot if we don't have a real name, could
> you let us know your name?

Yes, I will disclose my real name if needed. It doesn't seem to be needed just yet.

jdgordon writes:
> So please don't take our comments as offensive […]

No offense taken. But I have to admit that I was expecting a somewhat more welcoming attitude towards my first major contribution to Rockbox.

> My reasoning for trying to push you to making this work with the
> bookmark system is because (quite frankly) the current bookmark
> system sucks

Finally we agree on something… :)

> and can be done better and can add this feature simply.

…and here we disagree again. :)

I understand your motivation, but I won't be the one who fixes the bookmarking system. Rather, I am the developer who contributed a resume feature that is well designed and actually works. ;)
Comment by Alex Parker (BigBambi) - Thursday, 11 November 2010, 16:22 GMT
I use bookmarking extensively, and would love it to be better. However one of the major advantages of Rockbox in my eyes (and in many people's eyes) is that it does not force you to use a database system. I am loathe to lose this.

Incidentally, nothing was meant to be unwelcoming, and I'm sorry if it felt that way - it is just that the bar for new things to be accepted is (rightly) high. This means that if something is suggested that while being better than the current situation isn't as good as it could be, it probably won't get in.
Comment by MichaelGiacomelli (saratoga) - Thursday, 11 November 2010, 16:41 GMT
>This may be a misunderstanding. My resume feature works independently of whether you use the database or the file browser to start find tracks.

>The only precondition is that the database has been initialized, which I find to be an acceptable “limitation.”

This seems acceptable to me. Furthermore, the database seems like a logical place to store the information.


>I'm not a big fan at all of having to have the database initialised. The database being updated only if load to RAM is used is different,
>it is all within the same feature. Here we have to have a totally useless feature to me (the database) up and running to use a (what seems to be) unreleated one.

While I agree that the current database implementation is less then ideal, I think this is yet another reason the database needs improving, not a reason to work around storing metadata in the database. IMO the more the database is used the more likely it is to be fixed. The more its problems are hacked and worked around, the more problems we create.
Comment by Jonathan Gordon (jdgordon) - Friday, 12 November 2010, 01:05 GMT
"Yes, I will disclose my real name if needed. It doesn't seem to be needed just yet."
I want to commit the tagcache fixes :) you've emailed me your name so I can just get it from there unless you really dont want (which would be a shame)
Comment by sideral (sideral) - Friday, 12 November 2010, 07:28 GMT
> I want to commit the tagcache fixes :)

Excellent!

Could you please close the corresponding bugs if you do ( FS#11721 ,  FS#11722 , and  FS#11723 )?

> you've emailed me your name so I can just get it from there unless
> you really dont want (which would be a shame)

Please go ahead and use my name from my email for the time being. Of course I do want to have all of my contributions committed! :)
Comment by sideral (sideral) - Friday, 12 November 2010, 22:52 GMT
In an earlier comment I hinted that storing the resume offset in the database could also enable filtering by resume offset in database views. I just verified that my present patch already supports this feature.

For example, the following tagnavi_custom.config line adds a “Resumable podcasts” menu item that summons a list of all resumable tracks in the /PODCASTS directory, organized by album and formatted with a custom track-formatting rule (fmt_podcast):

"Resumable Podcasts" -> album ? filename ^ "/PODCASTS" & lastoffset > "0" -> title = "fmt_podcast"

As an aside, I don't quite understand what gripes people have with the database. From a user's perspective, I find Rockbox's database very convenient and flexible. I love how the database gets updated in the background (during playback) when I have added new files, and how I can customize the database views through the tagnavi_custom.config file. From a developer's perspective, I did not have much trouble either, except that there could have been more documentation. (On the other hand, I still haven't figured fully out how bookmarking works – probably because I seldom use the file browser for navigation.) In which way do you think is the database less than ideal and needs improvement?
Comment by MichaelGiacomelli (saratoga) - Monday, 15 November 2010, 00:31 GMT
Two main problems with the database come to mind:

1) Its somewhat buggy, and the underlying codecs it depends on often behave poorly with broken tags.
2) It uses a fair amount of RAM if you want autoupdated to work, which isn't a serious problem on most players, but can be a little ugly on some hard disk based players and also devices like the sansa clipv1 or old archos players that have extremely little RAM.

Comment by sideral (sideral) - Tuesday, 16 November 2010, 09:54 GMT
Thanks for your comments, saratoga!

The problems you've listed are certainly valid. Browsing through the mailing lists and forums, I could see that the DB is not universally loved, typically because of the second issue you've mentioned (DB takes up resources), and because many users do not derive much added value from the DB once they have neatly organized their files and folders.

Nonetheless, it also seems to be clear that many other users _do_ appreciate the DB, and view it as a major Rockbox feature. I do not believe that denying these users enhancements to the DB, or DB-powered new features (such as the one I am proposing), is of benefit to anyone. These users should not be held to ransom by the other group by insisting that every DB-enabled feature also be available without the DB. (The reverse should not be true either, and in fact it isn't: Bookmarks can be used only through the file browser, and not from DB views, for example.)

In this particular case, I contend that the feature I'm proposing can be implemented cleanly and efficiently only with the DB. Alas, users who elect not to use the DB cannot take advantage of it (or any of the other features the DB offers) and have to rely on a file-browser-enabled feature such as bookmarks – and vice versa.

I do not quite know what to make from the feedback I've received so far. The usefulness of the feature has not been questioned, but also no one stepped up to critique the actual implementation, to address the implementation issues I've raised, or to help find out what's missing to make the feature a commit candidate. I'm afraid without further encouragement I do not feel compelled to continue working on this feature. For me, it works and it is simple enough to forward-port to each release – may this be it, then?

One more comment:

saratoga writes:
> 1) Its somewhat buggy, and the underlying codecs it depends on often
> behave poorly with broken tags.

I agree that this complexity can make the DB less stable. However, the WPS uses much of the same metadata and relies on the same metadata-parsing code.

In my experience the Rockbox DB is vastly more stable than other players, including my Sansa ClipV2's and Clip+'s OFs, which would often ignore metadata or crash or freeze when trying to update its DB.

Flyspray lists 16 DB bugs (including the three I reported and provided fixes for), and about 8 bugs were filed on the bookmarking system. I'd say the defect rate for both systems is similar.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 November 2010, 11:02 GMT
Firslty I completely understand your sentiment.
Now the bookmark argument is a bit invalid because it was a feature before the database, so yea it should have been added with DB that happened years ago and no point arguing now. We generally don't like adding features which depend on others (I still reckon this can be done like cuesheets).

All that said. If the manual makes it clear this is limited to the DB AND it is configuarablle (an extra DB check every song is IMO a killer) them I would be open to accepting it.
Comment by MichaelGiacomelli (saratoga) - Tuesday, 16 November 2010, 23:26 GMT
>I agree that this complexity can make the DB less stable. However, the WPS uses much of the same metadata and relies on the same metadata-parsing code.


Yes, but during normal playback, only the files you decode actually impact stability. With the database, a single broken file can cause problems even if you never listen to it. Worse, since the database build does not provide feedback, theres no way to identify what the broken file actually is, so its extremely difficult to correct. This is yet another thing that needs improving in the database: some kind of logging that reports when problems are encountered and which files caused them.
Comment by Dave Slusher (GenioDiabolico) - Thursday, 18 November 2010, 17:50 GMT
Just for another data point, I downloaded the source and setup a development environment SPECIFICALLY so that I could build Rockbox with this patch. I had just tried it for the first time on my Sansa Clip+ last week. While it is highly cool overall, since 99.5% of my playtime is podcasts of up to 3 hours at a shot, losing place in those files because of fumbling on the skip buttons made it a drag (which I do fairly often.) If not for this patch, I'd have gone back to the OF.

I applied both of the above patches and ran them through the weekend. I've fiddled a bit with the code beyond that because I found the 20 minute heuristic on whether to treat like a song or podcast not helpful. Instead, I wrote a a function

bool is_podcast_or_audiobook(mp3entry*)

In there, I look for "/podcast" case insensitive in the path or "podcast" in the genre tag, and same for audiobooks. I call that function where the original length check was happening, and I also added it to the skip button as well, because when it is a left skip I don't want to rewind to 0:00 in podcasts but I do in songs. I did make one additional change, which is to reset the offset to 0 if the track change was automatic skip. Otherwise, if you played through you'd always resume at 1 second from the end. The final result is all the Rockbox goodness combined with Sansa's OF very nice handling of the /PODCAST hierarchy as special with the per track position preservation.

I'm happy to provide my patches as well if anyone cares. I'm kind of with sideral - this change makes Rockbox wildly more usable for me and it seems like there is pushback against it. I'm planning on running this set of changes indefinitely and I listen to podcasts all my working day, so it gets 6-10 hours of play every day. At least the Sansa Clip+ will be pretty well tested.
Comment by sideral (sideral) - Thursday, 18 November 2010, 22:21 GMT
GenioDiabolico,

Thanks a lot for trying and improving this patch and reporting your results!

Your enhancements seem to emulate the Clip+ OF's resume function really closely. I settled for the behavior I described partly because of laziness ;-) , but also because it generalizes the ability to resume to all tracks (not just podcasts and audiobooks) and allows skipping back to the beginning of a resumable track, which doesn't seem to be possible at all with the change you described.

Nonetheless, the behavior you implemented may be easier to grok for users than the 20-min heuristic, and it is also more robust against inadvertently skipping back to 0:00 and losing the resume point.

> […] I found the 20 minute heuristic on whether to treat like a song
> or podcast not helpful.

I actually do wonder why that is? The heuristic only kicks in on automatic track change… And for me, it has done the right thing every time without fail.

> I did make one additional change, which is to reset the offset to 0
> if the track change was automatic skip. Otherwise, if you played
> through you'd always resume at 1 second from the end.

My patch already does this (see the change to tagtree_track_finish_event); I wonder why that didn't work for you?

> I'm happy to provide my patches as well if anyone cares.

Yes, please do post your patches! Perhaps we can make a stronger patch with better default behavior and more configurability if we combine our efforts.

> this change makes Rockbox wildly more usable for me and it seems
> like there is pushback against it.

I actually read jdgordon's last comment as an invitation to come up with a commit candidate including the necessary manual changes and the ability to turn the feature off. I guess I'll eventually come up with such a patch, and your help testing and improving the feature is greatly appreciated. Right now I'm working with jdgordon on getting the tagcache fixes in.
Comment by Dave Slusher (GenioDiabolico) - Monday, 22 November 2010, 13:18 GMT
You are correct about the blanking at automatic skip. I looked at your original patch and I had screwed up that logic when I was making the tweaks with my function.

The reason why the 20 minute heuristic wasn't useful for me is that I have songs over 20 minutes and podcasts under 20 minutes on this player. It's good at a 90% approximation but I still want to keep my place in 18 minute podcasts.

So here is the patch for the changes I'm running on my Sansa Clip+. Note that they build on yours, so they MUST be applied after the set above. That seemed like it was the sensible thing to do since you already have them applied. I can regenerate them as a cumulative set that incorporates everything if that is more helpful. This is the first patch I've submitted to Rockbox so I'm not sure of all the ins and outs.

Thanks.
Comment by sideral (sideral) - Saturday, 27 November 2010, 23:27 GMT
Thanks for posting your patches! (The patch format is fine BTW; anyway, I could have coped either way (cumulative or incremental).)

I have been running your enhancements for a few days now, and I have to say that I like them, especially the skip-back behavior.

This brings us back to the question in which way the resume feature should be user-customizable. Here's a list of things that come to mind:

* Enable/disable the feature (independently from runtime-statistics gathering)
* Skip back behavior: Resume previous track, or jump back to current track's beginning
* Which tracks to resume when selecting track manually. All / filter by (configurable) directories / filter by (configurable) tags / filter both
* Which tracks to resume on automatic song change. All / filters (see previous item) / heuristic based on track length (configurable length threshold)

Thoughts?

Another open question is whether there needs to be a better refactoring. So far nobody took exception with the approach of restoring the resume position in tagtree_buffer_event. But as I outlined in a previous comment, this implies that tagtree_buffer_event has to run before the decoder can start its work. This does not make a difference on my player – but could it be a problem on more feeble players or players that use a real (non-SSD) hard drive?
Comment by Dave Slusher (GenioDiabolico) - Friday, 03 December 2010, 03:34 GMT
I made one more little tweak for my own fun. It was kind of a drag skipping forward past a few shows and having them resume a few seconds in later on. I made 15 seconds the floor for resuming, if less than that it resets to 0. I don't really have strong opinions, but it does make sense to have a "Turn on podcast saving" flag to enable the feature as a whole. The original firmware has a "Resume/reset" menu presented when you power on after having been in the middle of a podcast file.
Comment by sideral (sideral) - Monday, 06 December 2010, 18:44 GMT
Guys,

Here's my first commit candidate for this feature, comprising four patches (attached).

I have cleaned up the patches and made the feature configurable as requested and as outlined in an earlier comment. I'd be grateful if you could try this new patch and report whether the configuration options work for you. The autoresume menu is in the General Settings menu next to the bookmark settings. Remember that the default for this feature is _off_.

Please advice on the suitability of this patch for a merge. If you think it's good enough, I'll go ahead and add the necessary manual changes.

GenioDiabolico, I have included your latest change in this patch set as I find it pretty useful.

(I have not included a feature that allows asking the user for each manually selected track whether or not the track should be resumed. I have an experimental patch, but I couldn't quite get this to work right as I couldn't quite figure out in which thread the question should be asked to avoid any races and contentious behavior – and anyway, demand for this feature seems to be low. ;-)
Comment by Jonathan Gordon (jdgordon) - Tuesday, 07 December 2010, 09:42 GMT
pretty sure that first patch will break voice and lang files and needs to be done differently (I tihnk deprecate that value and add a new one, not sre though)

the code looks ok, but now its up to the point where you need to bring it up on the mailing list and see what sort of support there is for the complete patch
Comment by Alexander Levin (fml2) - Wednesday, 08 December 2010, 22:12 GMT
I've just committed the patch "0003-Fix-a-typo-in-a-comment.patch" as r28777. I wanted to include where I took the patch from into the commit message (i.e. this task number) but accidentally hit the enter key. Sorry for that.

I can't judge the quality of the patch since I absolutely don't know that part of code and also never use the database. And also don't listen to podcasts. So while I can't say anything pro or contra the patch, I wish you good luck in including it into the "official version". And thank you for your work!
Comment by MichaelGiacomelli (saratoga) - Saturday, 11 December 2010, 05:00 GMT
Committed 0002 in r28790.

I like the idea of this patch, but can't really comment on the implementation.
Comment by Jonathan Gordon (jdgordon) - Friday, 24 December 2010, 00:39 GMT
saratoga asked about the translation change...

Yes, you need to deprecate that LANG id and create a new one (it sucks I know, but not doing this will break lang and voice files for all non-lcd_sleep targets).
There is a command in langtool.py to do that deprecating. then just add the new string and fix the code to point to the new ID (I'd recomment LAND__NEVER with a comment in the english.lang file explaing the extra _ )
Comment by Christian Mertes (mudd1) - Wednesday, 29 December 2010, 17:10 GMT
It would be so great to see this in mainline soon!
Comment by MichaelGiacomelli (saratoga) - Saturday, 01 January 2011, 22:17 GMT
sideral:

Would you take a look at this version for an initial commit? I just disable the undecided menu options for now, but leave the code to implement them included. It works for me, but I haven't had as much time as I"d like to really dig into this due to work.

Edit: Just remembers this is going to fail on targets without backlight fading since they won't have the right lang string . . .
Comment by sideral (sideral) - Sunday, 02 January 2011, 00:41 GMT
Hi saratoga,

Thanks for coming up with your commit candidate!

As I wasn't sure whether the code for the features that were heavily debated on rockbox-dev could go in, I have been working on splitting the patch up by feature.

Additionally, I have slightly changed the rewind behavior, following a suggestion from Thomas Martitz: When rewinding to 0:00 using the Left key, the current playback position is saved as a resume point and preserved when the user presses Left again to skip to the previous track. This makes the prevent-rewind-to-zero feature mostly redundant.

The result is attached as a series of patches. These patches are relative to r28941 and supersede all earlier patches.

0001-Add-autoresume-feature.patch
adds the basic resume feature with a single on/off knob. Only manually selected tracks are resumed. This is what everyone agreed could be committed immediately.

0002-Do-not-update-resume-information-when-track-playback.patch
adds the 15s delay at track start in which the resume position is not updated, which allows skipping forward and backward across the track start without losing resume positions. I think this patch is quite uncontentious as well and could also be committed. However, there's still room for future improvement (see comments in the patch).

0003-Move-autoresume-setting-into-its-own-menu.-Precondit.patch
Moves the autoresume config option into its own menu (precondition for the following patches).

0004-Experimental-Make-the-phrase-Never-available-for-eac.patch
Experimental patch to make the phrase “Never” available for all targets (needed for next patch). jdgordon has commented that this should be done differently (deprecate old string, introduce new one), but I haven't quite figured out how exactly that should be done, hence I use this patch for the time being.

0005-Add-option-to-resume-next-track-on-automatic-track-c.patch
Adds option to resume next track on automatic track change

0006-Add-option-to-customize-which-tracks-should-be-consi.patch
Add option to customize which tracks should be considered as autoresumable (filter by filename or genre tag).
0007-Add-option-to-prevent-rewinding-to-the-beginning-of-.patch
Adds option to prevent rewinding to the beginning of a track to prevent loosing the current resume position. As I said, it's mostly redundant with patch 0002. I could live with this patch not making it into Rockbox.

0008-Fix-whitespace-error.patch
does what it says.
Comment by sideral (sideral) - Sunday, 02 January 2011, 01:04 GMT
Here's one more patch I've meant to add: Add Dave Slusher to the CREDITS file. You should credit Dave if you accept any one of patches 0002, 0006, or 0007.
Comment by sideral (sideral) - Sunday, 02 January 2011, 02:33 GMT
Argh, the first patch misses a "</voice>" in the lang file. Here's a replacement for 0001.
Comment by MichaelGiacomelli (saratoga) - Sunday, 02 January 2011, 02:49 GMT
>0001-Add-autoresume-feature.patch

Committed in r28942.
Comment by MichaelGiacomelli (saratoga) - Sunday, 02 January 2011, 03:52 GMT
Apparently this is kind of a fuck up on HWCODEC. I mistakenly thought HWCODEC used the same basic playback stuff as SWCODEC, but apparently this is not the case. Unfortunately I have no idea how HWCODEC works, and no device to try it on so I don't really know how to fix this.
Comment by sideral (sideral) - Sunday, 02 January 2011, 22:16 GMT
Sorry for the hassle, saratoga!

I think there's a small bug in your attempt to work around the HWCODEC issue. Patch attached.

As far as I have gleaned from IRC, porting autoresume to a HWCODEC device is not worth the effort because these devices do not have enough RAM to handle the database. Is this correct? I'd seriously consider porting the feature if I had access to such a device (or a sufficiently faithful simulation of one).

I'm also attaching a rebased version of patch 0002 in case you'd like to commit this one as well in short order. I'll eventually post a complete rebased patch set.

Please also consider patch 0008 — it's harmless, promise. ;-)
Comment by sideral (sideral) - Wednesday, 12 January 2011, 16:57 GMT
pixelma has committed patch 0001 (fix for r28943) in r29035
Comment by sideral (sideral) - Thursday, 13 January 2011, 23:43 GMT
In a recent IRC conversation [1], it was suggested to remove the string “requires initialized database” from the autoresume setting, and rather ask the user to initialize the DB in case it wasn't ready, with a splash prompt like “Error: Database is not initialized - Initialize now? Yes/No”.

Any comments? I don't mind either way and will comply with the request if no reason for not doing it pops up.

[1] http://www.rockbox.org/irc/log-20110112#16:14:28
Comment by sideral (sideral) - Friday, 21 January 2011, 23:45 GMT
Here's a summary of contention points left after a recent IRC conversation:

1. Base mechanism:

Llorean, and previously JdGordon, argue that the resume offset for each file should be stored using the bookmarking mechanism rather than in the DB, and storing them in the DB amounts to an unwanted duplication of mechanism.

While I can see the point in principle, I contend that basing autoresume on bookmarks as they exist today would be wrong (and I think JdGordon agrees with me here). The bookmarking system is a complex and fragile, and putting another feature on top is not going to make it better. Also, I see bookmarks and the DB as complementary. The DB is the right mechanism for storing per-file metadata, whereas bookmarks take snapshots of playlists. Storing per-file metadata in bookmarks would clutter the filesystem with bookmark files.

I'd be willing to port autoresume to a better base mechanism shared with bookmarks (possibly the one recently proposed by JdGordon), should one ever come along. The current bookmarking system isn't it.

I disagree that the feature should be held off until that new system is in place. There is no cost in enabling the feature now, and later saving the metadata elsewhere if so desired.

2. User interface / configuration complexity:

I prefer to allow one-time user configuration of which files should be resumable on automatic track change. Llorean prefers adding some UI functions (such as a special insert function) to enable autoresume for the next track on automatic track change. I cite ease of use as the major advantage of my proposal (no UI changes necessary except for some configuration), whereas Llorean cites ease of configuration and added flexibility.

I don't see why we can't support both. I'm not willing to sacrifice the automation and ease of use offered by my feature.

One recent proposal by Llorean is to add an attribute to some database views so that all playlists generated from that view would enable resume on automatic track change. Not a bad idea, but I really don't see the fundamental difference in flexibility and configurability to my present implementation, except that my implementation is easier to configure (through the configuration menu rather than through tagnavi_custom.config) and does not require starting autoresumable tracks from a DB view, but also from playlists, bookmarks, and from the file browser, which I find important for consistency.

3. Supporting audiobooks (not really a contention point)

The present patch set enables configuring resumability on manual track selection and on automatic track change only together, which isn't the right approach for audiobooks: for them, we want the first, but not the latter, as Llorean has pointed out. I agree. I suggest to keep the currently committed behavior of allowing manual resume for all tracks, but allowing configuration of which tracks to resume on automatic track change.
Comment by sideral (sideral) - Wednesday, 26 January 2011, 23:23 GMT
Here's my proposal for simplifying the configuration without loosing critical functionality.

The “Automatic resume” menu would look like this:

+ Automatic resume
+ + Enable automatic resume
+ + + Yes
+ + + No
+ + Resume on automatic track change
+ + + No
+ + + All tracks
+ + + Custom path/genre substrings (comma-separated)

Selecting “Yes” in “Enable automatic resume” checks for an initialized DB and prompts the user to initialize it if needed. Selecting “Custom path/genre…” fires the text editor to allow the user to enter the custom path/genre substring, which defaults to “podcast”.

We'd loose the following features, which I don't regard as core features:

* The prevent-rewind-to-0:00 feature. We don't need this any more now that we avoid overwriting the resume offset during the first 15s of the track.

* Customization of which tracks are autoresumable. Now all manually selected tracks can be resumed (this is the status quo as committed now). Only the set of tracks to be resumed on automatic track change can be customized.

* Separate customization of pathname and genre substrings. Now, one customization applies to both.

* The 20min heuristic for deciding on resume on automatic track change. The path/genre customization works just as well.

Comments?
Comment by sideral (sideral) - Sunday, 06 February 2011, 01:16 GMT
Here's an updated patch set that I intend to commit soon(ish). It implements the features and menu structure mentioned on my previous comment. Also, it removes the word “enable” in “enable automatic resume” to make the setting more consistent with other settings (an additional concern people have raised on IRC).

(NOT INCLUDED here is the skip-to-0:00 prevention (a patch I still maintain but do not intend to commit), and a new feature I have implemented that allows marking the currently playing track as “completed” by resetting its resume offset — nifty for generating a DB view listing (or excluding) all completed podcasts. I will post these patches separately when I'm done with the present ones.)
Comment by sideral (sideral) - Tuesday, 08 February 2011, 09:47 GMT
Updated patch 0001 with proper deprecation of old lang strings.
Comment by Paul Louden (Llorean) - Wednesday, 09 February 2011, 00:08 GMT
It seems to me substring matching makes it far too easy to match accidental values.

I feel it'd be better to only match exact strings, so that "podcasts, /podcasts" will be necessary if you want the podcasts genre and folder. It means slightly more typing for the file, but much less chance of misbehaviour, and shouldn't be an overly significant burden at initial setup.
Comment by sideral (sideral) - Wednesday, 09 February 2011, 22:31 GMT
I can see why matching whole words only would probably be better.

However, I dislike requiring a “/” for filepath matches. If we really want to discern genre from filepath matches, we should reintroduce separate settings for matching genres and filepaths, respectively. That would be more self-describing from them name of the setting, and thus more discoverable for users. If we're not ready to introduce this much configuration complexity, then the “/” magic has no place either.

Question to the language-system experts: When I change the setting to match complete words only, I'd like to rename it from “Custom path/genre substrings (comma-separated)” to “Custom paths/genres (comma-separated)”. If I manage to do this before the 3.8 feature freeze and before any translator has catched up with the previous name, can I just change the language string or should I properly deprecate it? (I assume that each deprecated string takes up some resources in the final binary; let me know if this is not the case.)
Comment by sideral (sideral) - Wednesday, 09 February 2011, 22:34 GMT
PS, just for the record: The most recently posted patches 0001–0003 have been committed in r29249–r29252.
Comment by Paul Louden (Llorean) - Wednesday, 09 February 2011, 22:52 GMT
If you need "audio/podcasts/" if the folder is going to be in a subfolder, why should the initial / be left off?

Basically, you didn't exactly give a reason not to have the initial / since it'll be necessary for subfolders of any sort other than that. It's not "magic" it's just typing the whole path.
Comment by sideral (sideral) - Wednesday, 09 February 2011, 23:11 GMT
Llorean, thanks for your response.

I may have misunderstood your original proposal. I first thought you were asking for complete word matches only (so that, for example, a search term “/podcast” would match any file in “/audio/podcast/…” but not in “/audio/podcasts/…”).

You actually seem to suggest that the complete dirname (anchored to the root directory) must match (so “/podcast” matches “/podcast/…” but not “/audio/podcast/…”), in which case the leading “/” makes sense. (Would “/podcast” also match “/podcasts” (mind the “s”)?)

I'm not sure which proposal I like better. I think I tend towards the one outlined in my previous comment (whole-word match on genre or any path component) for its simplicity.
Comment by Paul Louden (Llorean) - Wednesday, 09 February 2011, 23:14 GMT
I think /spoken should cover /spoken/podcast (if you get a folder, include its subdirectories) but not /podcast/spoken. Basically, you should be able to include a folder and get all subfolders and tracks within that folders, but the match must still be very explicit.

Basically the idea is to eliminate, as much as possible, the potential for false positives. So yes, the complete dirname must match, or it must be contained within a matched directory.
Comment by Steve Bavin (pondlife) - Thursday, 10 February 2011, 07:51 GMT
Coming late to the party here, could we not have a marker file in a "podcast" directory - akin to database.ignore (i.e. database.podcast) ? Then no hard-coded names would be needed. (I don't like hard-coded names...!)
Comment by sideral (sideral) - Thursday, 10 February 2011, 10:05 GMT
Llorean, I think you are right. I'll change this to match full dirnames only. A recent IRC discussion came to the conclusion that matching the genre tag is considered too fragile, and I'll remove that entirely.

pondlife, thanks for chiming in! This discussion isn't about hard-coded names – quite the opposite: This is about a configuration option that allows you to specify the directories in which tracks can be resumed on automatic track change [1]. Using a key file rather than a config option for this purpose has been proposed a few times, but I have rejected that idea because it makes the feature nondiscoverable, and also because the directory hierarchy would have to be consulted on each track playback (and not just once on DB rebuild, unless we'd want to waste space for this in the DB).

[1] as opposed to manual track selection, for which there is a global configuration option only
Comment by Alex Parker (BigBambi) - Thursday, 10 February 2011, 12:58 GMT
Paul said:
"I think /spoken should cover /spoken/podcast (if you get a folder, include its subdirectories) but not /podcast/spoken. Basically, you should be able to include a folder and get all subfolders and tracks within that folders, but the match must still be very explicit."

I agree with this. It means you can be very precise about what you match, without having to be careful for surprise unintended matches.
Comment by sideral (sideral) - Friday, 11 February 2011, 07:53 GMT
The full-dirname-matching-only behavior has been committed as r29280.
Comment by sideral (sideral) - Tuesday, 01 March 2011, 14:43 GMT
Split out rewind-to-0:00 prevention patch to FS#11977.

Loading...