Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9114 - Pause Between Tracks

Attached to Project: Rockbox
Opened by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 24 June 2008, 12:22 GMT+2
Task Type Patches
Category Applications
Status New
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

This patch adds a new playback option, Pause Between Tracks. It pauses the playback automatically after each track, and the user must press Play to play the next.

This is useful for e.g dance classes, sound effects etc.

There is a noticeable lag in the simulator, where it starts the playback of the next song before pausing, but who cares? It seems to work fine on the target anyway.

Try it!
This task depends upon

Comment by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 24 June 2008, 12:22 GMT+2
...and here's the patch.
   pause_between_tracks.patch (6 KiB) 
 apps/lang/english.lang     |   14 ++++++++++++++
 apps/settings.h            |    1 +
 apps/menus/playback_menu.c |   12 +++++++-----
 apps/settings_list.c       |    2 ++
 apps/playback.c            |   37 +++++++++++++++++++++++++++++++++++++
 apps/playback.h            |    3 +--
 apps/SOURCES               |    2 +-
 apps/main.c                |    3 +++
 firmware/export/events.h   |    1 +
 firmware/mpeg.c            |    1 +
 10 files changed, 68 insertions(+), 8 deletions(-)

Comment by Marianne Arnold (pixelma) - Tuesday, 24 June 2008, 13:03 GMT+2
Maybe I'm misunderstanding something (but don't think because compiling for my OndioFM fails "In file included from playback.c:42: codecs.h:222: warning:..." etc.) but it looks like you enabled compiling playback.c on hwcodec (in apps/SOURCES).

Edit: It only gives the warning, failing had another reason).
Comment by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 24 June 2008, 13:21 GMT+2
Yes, it is supposed to compile playback.c, because I didn't want to duplicate the event handling code.

I'll fix the warnings, though.
Comment by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 24 June 2008, 13:25 GMT+2
Here is an updated version, without warnings.
   pause_between_tracks-2.patch (6.5 KiB) 
 apps/lang/english.lang     |   14 ++++++++++++++
 apps/settings.h            |    1 +
 apps/menus/playback_menu.c |   12 +++++++-----
 apps/settings_list.c       |    2 ++
 apps/playback.c            |   39 +++++++++++++++++++++++++++++++++++++++
 apps/playback.h            |    3 +--
 apps/SOURCES               |    2 +-
 apps/main.c                |    5 +++--
 firmware/export/events.h   |    1 +
 firmware/mpeg.c            |    1 +
 10 files changed, 70 insertions(+), 10 deletions(-)

Comment by Steve Bavin (pondlife) - Tuesday, 24 June 2008, 18:04 GMT+2
Maybe the event handling code could be moved to another module?

I'd personally/selfishly prefer to keep playback.c and HWCODEC apart, because I have no HWCODEC target to test with.
Comment by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 24 June 2008, 22:21 GMT+2
Maybe. Care to modify the patch accordingly?
Comment by Steve Bavin (pondlife) - Wednesday, 25 June 2008, 10:52 GMT+2
Of course, but probably not for a week or 2 ;-)

Real Life is such an intrusive thing.
Comment by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 05 August 2008, 11:10 GMT+2
The question is where to move it, since the code really belongs in the playback module. I could of course duplicate the code...
Comment by Rosso Maltese (asettico) - Monday, 03 August 2009, 13:13 GMT+2
Resync'd against r22137.
   apps-9114pause_between_tracks-3.0.patch (6.3 KiB) 
 apps/lang/english.lang     |   14 ++++++++++++++
 apps/settings.h            |    1 +
 apps/menus/playback_menu.c |    4 +++-
 apps/appevents.h           |    1 +
 apps/mpeg.c                |    1 +
 apps/settings_list.c       |    2 ++
 apps/playback.c            |   39 +++++++++++++++++++++++++++++++++++++++
 apps/playback.h            |    3 +--
 apps/SOURCES               |    2 +-
 apps/main.c                |    5 +++--
 10 files changed, 66 insertions(+), 6 deletions(-)

Comment by Bryan Childs (GodEater) - Tuesday, 15 December 2009, 09:57 GMT+2
Resync'd against 24003.
   pause_between_tracks_24003.diff (6.4 KiB) 
 b/apps/SOURCES               |    2 +-
 b/apps/appevents.h           |    1 -
 b/apps/lang/english.lang     |   14 --------------
 b/apps/main.c                |    5 ++---
 b/apps/menus/playback_menu.c |    4 +---
 b/apps/mpeg.c                |    1 -
 b/apps/playback.c            |   38 --------------------------------------
 b/apps/playback.h            |    1 -
 b/apps/settings.h            |    1 -
 b/apps/settings_list.c       |    2 --
 10 files changed, 4 insertions(+), 65 deletions(-)

Comment by Bryan Childs (GodEater) - Tuesday, 15 December 2009, 10:25 GMT+2
Added a check to make sure the pause doesn't happen if the DAP has party mode set.
   pause_between_tracks_24003_2.diff (6.4 KiB) 
 b/apps/SOURCES               |    2 +-
 b/apps/appevents.h           |    1 -
 b/apps/lang/english.lang     |   14 --------------
 b/apps/main.c                |    5 ++---
 b/apps/menus/playback_menu.c |    4 +---
 b/apps/mpeg.c                |    1 -
 b/apps/playback.c            |   39 ---------------------------------------
 b/apps/playback.h            |    1 -
 b/apps/settings.h            |    1 -
 b/apps/settings_list.c       |    2 --
 10 files changed, 4 insertions(+), 66 deletions(-)

Comment by Rosso Maltese (asettico) - Tuesday, 15 December 2009, 11:15 GMT+2
Ops, I did a re-sync last Sunday, but I forgot to upload it, sorry.

I not completely agree with the last check: also during a party I would like to play a track and pause at the end.
I think the 2 settings could survive independently. ;-)
Comment by Bryan Childs (GodEater) - Tuesday, 15 December 2009, 11:28 GMT+2
But if the track pauses whilst you're in party mode, there's no way to get the player to advance to the next track without leaving party mode. Why would you want this behaviour?
Comment by Rosso Maltese (asettico) - Tuesday, 15 December 2009, 15:00 GMT+2
Uhmmm... interesting question. :-) Which behaviour should be stronger?
If party mode wins, then pause between track should be disabled; this means that it should disappear from the related menu (I didn't remember of any menu entry showed as "disabled").
If pause between tracks wins, then it should enable the only key(s) to allow the desired behaviour (just play, I suppose).
But who decide which will be stronger? The developer, a poll, or YACS (yet another configuration setting)? ;-)
Comment by Bryan Childs (GodEater) - Tuesday, 15 December 2009, 15:04 GMT+2
Well, myself, Llorean and LinusN all think party mode should take precedence over this setting. We don't have to have the menu item appear as disabled though, so long as we document this behaviour and it is understood.
Comment by Rosso Maltese (asettico) - Tuesday, 15 December 2009, 15:21 GMT+2
And what about a three value setting instead of o boolean?
Something like: off, on but in party mode, always on.
Comment by Bryan Childs (GodEater) - Tuesday, 15 December 2009, 15:42 GMT+2
Party mode is designed to give you uninterrupted music at a party. It's supposed to stop people fiddling with the controls so that the music doesn't stop when someone's looking at the player and pressing buttons inadvertently. You must go to some weird parties if you want the music to stop after every song and have a very experienced Rockbox user return to the player to get it to move to the next track.

In my opinion Party mode should take precedence over *everything* which interferes with music playback being continuous.
Comment by Frank Gevaerts (fg) - Tuesday, 15 December 2009, 15:44 GMT+2
I think this should be kept simple, with party mode overriding everything.

I can see a use for a mode where any track that starts playing should not be interrupted, which could coexist (and be useful) with pause between tracks, but I don't think that's the same as party mode.
Comment by Steve Bavin (pondlife) - Tuesday, 15 December 2009, 16:04 GMT+2
Getting way off-topic for this patch, but this is a decent example of why I'd like party mode to be replaced with (or redefined as) keylock mode...
Comment by Bryan Childs (GodEater) - Tuesday, 15 December 2009, 16:14 GMT+2
@pondlife, please bring that discussion to IRC ;)
Comment by Bryan Childs (GodEater) - Wednesday, 16 December 2009, 13:38 GMT+2
Resync'd against 24024, and fixed compilation so it would work on HWCODEC.
   pause_between_tracks_latest.diff (6.6 KiB) 
 b/apps/SOURCES               |    2 +-
 b/apps/appevents.h           |    1 +
 b/apps/lang/english.lang     |   14 ++++++++++++++
 b/apps/main.c                |    5 +++--
 b/apps/menus/playback_menu.c |    4 +++-
 b/apps/mpeg.c                |    1 +
 b/apps/playback.c            |   39 +++++++++++++++++++++++++++++++++++++++
 b/apps/playback.h            |    2 ++
 b/apps/settings.h            |    1 +
 b/apps/settings_list.c       |    2 ++
 10 files changed, 67 insertions(+), 4 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Thursday, 17 December 2009, 09:47 GMT+2
I'm currently against commiting that patch. playback.c really should not be compiled for hwcodec and the added functions there don't really deserve to be in playback.c anyway. That fil is crowded enough as it is, and they dont really have anything to do with the playback engine.

so the needed changes:
1) get the callback/event stuff out of playback.c, either into a new file or I tihnk playlist.c makes more sense (its a playlist mode just as much a playback mode... or even in the menus file and fix the setting callback to do the initialisation)
2) change audio_pause() to audio_pause(bool block) which calls queue_post() instead of queue_send() so both hwcodec and swcodec can use the same codepath in the event callback (I have no idea why audio_pause() currently blocks...
Comment by Steve Bavin (pondlife) - Thursday, 17 December 2009, 10:02 GMT+2
Ow, I hadn't noticed that.

I don't think playback.c should need changing for this feature - assuming there's already an end-of-track event you can use (i.e. aftert the PCM output is done, might need to check how crossfade reacts too).

This is more a UI-level feature in terms of the code structure, I'd think. Maybe it could go in playlist.c, though that's a bit of a mess (or was last time I looked).

Sorry - not wishing to be a monstrous killjoy, just wishing to keep the code structured right.. I'm happy to have a look at this over Christmas week if needed (and I can get near a PC).
Comment by Mike Schmitt (Falco98) - Tuesday, 12 January 2010, 09:38 GMT+2
Linus, this patch seems to share a lot of functionality with what would be needed to implement a tool whereby someone can long-select a track name in the 'current playlist' and select a "stop after this song" option from the menu there. That is to say, both require a system to be in place where the player plans to stop at the end of its currently-playing track (at some point), even though it's not at the end of the playlist yet. Would you be willing to make such an addition here, or at least keep such a possibility in mind while finalizing the design of the functionality of this one, so the processes introduced here could be used to help make the feature I'm talking about?
Comment by Rosso Maltese (asettico) - Friday, 12 November 2010, 15:38 GMT+2
Synced against r28563.
It works, but it has a bug: if pause between tracks is enabled, inserting something in the playlist when a track is playing, it pauses current track, even if the end is not reached.
   9114-apps-Pause_between_tracks.v6.1.0.patch (6.2 KiB) 
 apps/lang/english.lang     |   14 ++++++++++++++
 apps/settings.h            |    1 +
 apps/menus/playback_menu.c |    4 +++-
 apps/mpeg.c                |    1 +
 apps/settings_list.c       |    2 ++
 apps/playback.c            |   39 +++++++++++++++++++++++++++++++++++++++
 apps/playback.h            |    2 ++
 apps/SOURCES               |    2 +-
 apps/main.c                |    5 +++--
 9 files changed, 66 insertions(+), 4 deletions(-)

Loading...