Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11619 - Restore pitch and speed settings on resume

Attached to Project: Rockbox
Opened by Frank Gevaerts (fg) - Friday, 10 September 2010, 19:49 GMT+2
Last edited by Frank Gevaerts (fg) - Friday, 10 September 2010, 20:07 GMT+2
Task Type Patches
Category Music playback
Status New
Assigned To No-one
Player Type SW-codec
Severity Low
Priority Normal
Reported Version Release 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

This patch should save pitch and speed settings to nvram, and restore them after reboot if needed.

If I read the code correctly, there should be enough room in nvram (and anyway, the way this is done doesn't apply to any targets that actually use real nvram)

This code compiles, but apart from that it's completely untested.

Of course, since this patch increments the nvram data version, you'll lose resume info the first time you reboot.
   timestretch_v1.diff (3.2 KiB) 
 apps/playlist.c      |   13 +++++++++++--
 apps/settings.h      |    5 +++++
 apps/settings_list.c |    4 ++++
 apps/settings_list.h |    2 +-
 apps/root_menu.c     |    4 ++++
 5 files changed, 25 insertions(+), 3 deletions(-)

This task depends upon

Comment by Frank Gevaerts (fg) - Friday, 10 September 2010, 20:02 GMT+2
Missed a warning
   timestretch_v2.diff (3.3 KiB) 
 apps/playlist.c      |   15 +++++++++++++--
 apps/settings.h      |    5 +++++
 apps/settings_list.c |    4 ++++
 apps/settings_list.h |    2 +-
 apps/root_menu.c     |    4 ++++
 5 files changed, 27 insertions(+), 3 deletions(-)

Comment by Jens Arnold (amiconn) - Saturday, 11 September 2010, 11:28 GMT+2
Why nvram? If pitch and time stretch were to be made persistent, a setting would be better choice imo.
However, when pitch (and later time stretch) got added, they were not made persistent on purpose.
Comment by Frank Gevaerts (fg) - Saturday, 11 September 2010, 13:09 GMT+2
I know they're not persistent on purpose, but my understanding of why that's the case is that they are really specific to the audio you're listening to at a given moment, and in most cases not suitable for any other audio.

I fully agree with that reasoning, which is why I didn't make them a setting. They're only restored if playback is resumed from the position it was at at shutdown, which I think still matches the requirements of not being "too" permanent.

Actually, I suspect it might be a good idea to reset pitch and speed when starting a new playlist. That (together with this patch) would provide the most consistent behaviour: pitch and speed belong to the current playlist, *nothing* else. That also fits in nicely with them being stored in bookmarks.
Comment by Brian Hall (brihall) - Monday, 13 December 2010, 19:46 GMT+2
This patch has been working great for me the last two weeks. Built rockbox from svn specifically to use this patch. Saves me several minutes of my time per day, since I no longer have to set my desired playback speed for podcasts prior to starting my commute. I just power up and go. Thanks!

Seems like a good candidate for merging into mainline to me.
Comment by Alexander Levin (fml2) - Wednesday, 05 January 2011, 16:19 GMT+2
With the reset when starting a new playlist implemented, it would be an ideal candidate for inclusion into SVN. IMHO.
Comment by Rosso Maltese (asettico) - Friday, 22 July 2011, 15:05 GMT+2
Sync to r30189.
   11619-playback-Restore_pitch_and_speed_settings_o... (3.4 KiB) 
 apps/playlist.c      |   14 ++++++++++++--
 apps/settings.h      |    5 +++++
 apps/settings_list.c |    4 ++++
 apps/settings_list.h |    2 +-
 apps/root_menu.c     |    4 ++++
 5 files changed, 26 insertions(+), 3 deletions(-)

Comment by Frank Gevaerts (fg) - Sunday, 31 July 2011, 15:19 GMT+2
I agree that reset when starting a new playlist would be nice, but on the other hand it's been pointed out to me that it's useful to be able to set pitch/screen before starting a playlist (which you can do now, so breaking that would be a regression).

That means the ideal logic would be to only reset if pitch/speed haven't been changed since the last time playbacvk was active, which is slightly more tricky.
Comment by Brian Hall (brihall) - Tuesday, 27 September 2011, 00:13 GMT+2
I'd been using the older timestretch_v2.diff patch for awhile, but I wanted to rebuild with the current RB and use the new 11619 patch. Built fine and I've installed it, but now I can't get timestretch to work on the pitch screen- it doesn't let me increase playback speed by using left/right like it did before. I can use the dial to increase speed, but it doesn't maintain the pitch at all. I have toggled Sound Settings->Timestretch on and off (Yes/No) via the Context Menu and rebooted several times, but I can't get the old behavior with the new patch. This is on the Sansa E200R, svn r30602M-110925.

Is there something special I need to do after upgrading to the new patch? Process was, applied timestretch_v2.diff in reverse to remove it from my tree, make clean, svn update, apply 11610 patch, rebuild.
Comment by Brian Hall (brihall) - Friday, 14 October 2011, 16:18 GMT+2
Works again for me as of svn r30738M-111009

Comment by Zachary (Hawkwing) - Tuesday, 17 July 2012, 00:45 GMT+2
This patch, or something similar, should really be integrated into Rockbox. Having to reset the speed after startup is very annoying, and if it's the same audio playing as it was before shutdown there's no reason for it to play at a different speed.

Loading...