Rockbox

Tasklist

FS#11931 - Do a short rewind when playback is paused

Attached to Project: Rockbox
Opened by John Morris (JohnMorris) - Sunday, 13 February 2011, 05:00 GMT
Last edited by sideral (sideral) - Friday, 13 May 2011, 23:03 GMT
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.7.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

[EDIT: The scope of this patch has evolved over the discussion below. I'm updating the description for the benefit of the casual lurker. The description refers to the latest patch revision posted in the comments section. –sideral]

This patch adds an option to rewind the current track by a few seconds when it is paused. This is useful for audiobooks and podcasts to allow the listener to recall where the track was left off.

The patch subsumes the existing rewind-on-headphone-unplug feature and extends it to all invocations of pause: play/pause button (or touch area), headphone removal, and power-supply unplug in car-adapter mode. It also subsumes  FS#9448  (rewind on fade).

In passing, this also makes the fade in/out behavior more consistent across all pause causes. Previously there was no fade-in on headphone insertion (which seems like a particularly good place to have fade in, BTW), nor on unpause on car adapter mode. [EDIT: This part has been committed as r29844/r29845.]

As a result of this refactoring, this patch actually slightly reduces the number of lines of C source code.

[John Morris' original description of his originally submitted patch follows:]

This short patch adds an automatic rewind when in car-adapter mode. When the power goes - the car is stopped - off the track will be rewound by ten seconds (or to the start of the track if that's less than ten seconds). This is particularly effective for audiobooks, to recall where the story left off.
This task depends upon

Closed by  sideral (sideral)
Friday, 13 May 2011, 23:03 GMT
Reason for closing:  Accepted
Additional comments about closing:  The feature (patch 0002) has been committed (along with the necessary manual changes) as r29876–r29878. Thanks for doing the hard part of the work, John Morris!
Comment by John Morris (JohnMorris) - Sunday, 13 February 2011, 05:03 GMT
Edit: forgot to include the patch...
Comment by sideral (sideral) - Sunday, 13 February 2011, 23:27 GMT
This is related to  FS#9448  (rewind on fade), and should be unified with it towards a more general rewind-on-resume feature, which I would welcome. In my view, rewind on resume should depend on neither car-adapter mode nor fading.

A recent IRC conversation showed a preference to have separate settings for the rewind amount for rewind on resume after pause and rewind on resume after stop, but I don't know how representative this was. I don't have a preference and could also live with just one rewind setting that applies to both unpause and resume after stop.
Comment by John Morris (JohnMorris) - Monday, 14 February 2011, 15:46 GMT
Please taks a look at this patch.

I found four causes for a pause:

- pressing the play/pause button
- external power off in car-adapter mode
- headphone removal (where supported by hardware)
- touch screen play/pause

This patch adds "rewind on pause" in all of those. However, I could only test the first two with the players I have.

The rewind duration is the same for all pause causes (but, from reading the earlier discussion, independent of the fade in/out duration).

Looking at the existing pause code I saw several instances of the idiom "if (fade_enabled) fade(); else pause();". But this was not applied consistently for the four pause causes. So I've added two new functions "auto_pause" and "auto_resume" to wrap up the fade and auto-rewind functionality in one place.

I re-purposed the existing rewind on headphone removal config variable and menu item tag for this - since the same rewind duration is used for all pauses, that is no longer needed.

At the moment it is added to the end of the playback menu, and defaults to zero - no rewind.

In passing, this also makes the fade in/out behavior more consistent across all pause causes. Previously there was no fade-in on headphone insertion (which seems like a particularly good place to have fade in, btw), nor on unpause on car adapter mode.

As a result of all this, this patch actually slightly reduces the number of lines of source code. More functionality for less code - I love it when that happens :)




Comment by sideral (sideral) - Tuesday, 08 March 2011, 14:09 GMT
Thanks! At first sight, your patch looks fine. (Lang changes need to be done slightly differently to not break existing translations and voice files, but that's a minor issue that we can fix before committing.)

I'll try your patch for a while and will report back with my experiences.


Comment by John Morris (JohnMorris) - Wednesday, 09 March 2011, 03:32 GMT
Ah - I knew I'd mess something up! If there's guidance on how to do the language changes I'll re-work that aspect.

I've been using it myself since writing it, and find it perfect for audiobooks in the car. With this and "resume play on power on" configured I never have to touch the player at all from one commute to the next, except when reaching the end of a book and picking the next one.
Comment by sideral (sideral) - Wednesday, 09 March 2011, 14:14 GMT
Lang items aren't simply changed, but deprecated and replaced by new ones. Normally I'd point you to tools/langtool.pl --deprecate, but unfortunately that doesn't work quite right at the moment. I've filed a bug at  FS#12003 , which also explains how deprecation needs to be done AFAIK. You can also look at some examples in the english.lang file, such as LANG_AUTORESUME_ENABLE, which was recently deprecated.

Here are some other minor things to look out for when you update your patch:
* Have you tried compiling your code for various targets, including a HWCODEC target such as the Archos Ondio FM?
* I suggest to rename "pause_rw" to "pause_rewind" to align the name with "resume_rewind".
* Also, "auto_resume" sound confusingly similar to the various symbols related to the autoresume feature that already exist. I suggest renaming the "auto_pause/resume" functions to something more descriptive.
Comment by John Morris (JohnMorris) - Friday, 11 March 2011, 05:52 GMT
Try this one. Functionally the same and working ok on my Sansa Fuze V2.

* Added a new lang item and deprecated the old one instead of replacing it.
* Did a test build for Archos Ondio FM (ok)
* "pause_rw" ==> "pause_rewind" (code and lang)
* "auto_pause / auto_resume" ==> "pause_action / unpause_action" - I decided to avoid the "resume" word altogether as it is getting too overloaded.
Comment by sideral (sideral) - Friday, 18 March 2011, 22:02 GMT
Thanks for the update, John!

I've been running with this patch for a while, and it works perfectly for me. Also, your latest revision looks like it's almost ready for commit (I've locally made a few whitespace changes to adapt your code to the existing style).
Comment by sideral (sideral) - Friday, 18 March 2011, 22:24 GMT
Before I bring this up for discussion for a possible commit, I'd like to quickly discuss two possibly contentious design decisions – just to be prepared when I'm interrogated by the inquisition. :)

1. What's the advantage of having rewind on stop and rewind on pause as two features that can be enabled and configured separately?

My personal answer is that I prefer rewind on stop to rewind a little further; I use rewind on pause to make up for the fading delay and to refresh my short-term memory of where I was in the track.

In a recent IRC discussion, some people made a point of pause and stop being two separate features and that some users prefer them to work differently. I don't share this particular sentiment, but this patch respects it.

2. What's the advantage of doing the rewind on pause rather than on unpause?

I personally don't mind, but have noticed that this can result in a somewhat odd interaction with the rewind-on-stop feature, which rewinds the track further and in addition to the pause rewind.
Comment by John Morris (JohnMorris) - Saturday, 19 March 2011, 04:53 GMT
Thanks, Sideral.

OK on the white space: I'll try harder next time :)

I have no opinion on (1) - these days I hardly ever do use stop. I just pause and let the player power itself off after a while, unless I un-pause. In the car, if the player has powered off I have it configured to go to WPS when the power comes back. Net effect is the optimum in automation - I never touch it from the start to the end of a book. One to two weeks worth of commute time typically. For me rewind on pause was a "scratch my own itch" thing. As per your own comment of 13 February, I could live with one setting if that was the consensus.

On (2) I don't think it matters either way: audibly the effect is the same, at least in my usage mode. The only difference is when the track time on the WPS changes. I did look at both ways and doing it on pause looked at the time - and after looking at the existing code for rewind after stop - to be simpler to implement.



Comment by sideral (sideral) - Tuesday, 05 April 2011, 12:33 GMT
Rebased patch to r29679.
Comment by sideral (sideral) - Tuesday, 03 May 2011, 08:18 GMT
Use audio_pre_ff_rewind before doing the actual rewind to prime pcmbuf and flush audio prior to rewinding. This became necessary with the recent playback-engine renovation. (Thanks jhMikeS for suggesting this!)

Also: Do not call audio_ff_rewind when we do not actually rewind. Plus minor whitespace changes.

[EDIT: Updated patch to make it compile with HWCODEC. Added CREDITS. ]
Comment by sideral (sideral) - Friday, 06 May 2011, 22:23 GMT
Split patch into two: One for bug-fixing fade in/out only, and the 2nd one with the actual new feature.
Comment by sideral (sideral) - Monday, 09 May 2011, 13:15 GMT
Patch 0001 has been committed as r29844/r29845. Patch 0002 has been rebased to that revision (attached).

Loading...