Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#5344 - Separate "Shuffle" and "Track Skip" settings for Crossfade

Attached to Project: Rockbox
Opened by Mike Schmitt (Falco98) - Thursday, 11 May 2006, 23:43 GMT+2
Last edited by Zakk Roberts (midkay) - Thursday, 18 May 2006, 07:55 GMT+2
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Proposal: Separate the options in "Crossfade Settings" for "track skip" and "shuffle".

Currently you can select "shuffle", "track skip only", "always", and "never".
What if i want "shuffle" AND "track skip"? when i have Shuffle on, i want to have every song change crossfaded, but when i have regular or album play on, i don't (necessarily) want crossfade unless i skip a track. Currently, switching back and forth between "shuffle" and listening to an album (or intentional playlist which i don't want crossfaded) requires that i change the CF settings too. if there were a "shuffle and track skip" option, or both options were selectable separately, it would take some hassle out of it. Plus, the code is already there, so it should be easy to implement.

After this is implemented, a few easy modifications could be made to allow for separate crossfade settings for "track skip" versus regular crossfading (see http://www.rockbox.org/tracker/task/4953 ).
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Tuesday, 19 December 2006, 02:26 GMT+2
Reason for closing:  Accepted
Comment by David Brown (ep0ch) - Tuesday, 16 May 2006, 12:16 GMT+2
agreed i really would like to see a "shuffle and track skip" option for crossfade
Comment by Mike Schmitt (Falco98) - Thursday, 18 May 2006, 07:19 GMT+2
Ok, so i've just gone ahead and made a patch to add "Shuffle AND Track Skip" to the "crossfade enable" menu. My preferred implementation (described above) would be much more involved to do, and may even necessitate rewriting crossfade significantly... for now, this does what *I* want, so i'm happy =)

everyone test, let me know what you think :)
(application/octet-stream)    track_skip_shuffle.patch (4.3 KiB) 
 playback.c      |   11 +++++++++--
 settings.c      |    4 ++--
 settings.h      |    6 ++++--
 settings_menu.c |    3 ++-
 4 files changed, 17 insertions(+), 7 deletions(-)

Comment by Rani Hod (RaeNye) - Thursday, 18 May 2006, 12:53 GMT+2
Haven't tested it yet, but I like the idea.
Didn't you change anything in lang/english.lang as well?
(where is LANG_SHUFFLE_TRACKSKIP #defined?)
Comment by Mike Schmitt (Falco98) - Thursday, 18 May 2006, 16:48 GMT+2
Whoops. I had done it, but forgot to copy the file into the source structure from which i took the diff.
Here's an updated one with the change to english.lang included.
(application/octet-stream)    track_skip_shuffle_B.patch (5 KiB) 
 lang/english.lang |   14 ++++++++++++++
 playback.c        |   11 +++++++++--
 settings.c        |    4 ++--
 settings.h        |    6 ++++--
 settings_menu.c   |    3 ++-
 5 files changed, 31 insertions(+), 7 deletions(-)

Comment by Mike Schmitt (Falco98) - Thursday, 18 May 2006, 23:52 GMT+2
Whoops again: just found a big error in my code (actually just a big misunderstanding by me) regarding how automatic-track-changes are crossfaded (or not), such that it crossfaded even if "shuffle" was off (which goes against the whole point of my patch).

New version: should completely solve that; it now track-changes exactly how it does when crossfade is set to "shuffle" but "shuffle" is set to OFF. (except, of course, it will also crossfade when you manually track-skip no matter what the mode.)
(application/octet-stream)    track_skip_shuffle_C.patch (5.4 KiB) 
 lang/english.lang |   14 ++++++++++++++
 playback.c        |   19 +++++++++++++++++--
 settings.c        |    4 ++--
 settings.h        |    6 ++++--
 settings_menu.c   |    3 ++-
 5 files changed, 39 insertions(+), 7 deletions(-)

Comment by Mike Schmitt (Falco98) - Friday, 19 May 2006, 15:53 GMT+2
Ok, this time I've just fished out one little bug that made it act weird. This *MAY* be the last update :)
(application/octet-stream)    track_skip_shuffle_D.patch (5.5 KiB) 
 lang/english.lang |   14 ++++++++++++++
 playback.c        |   22 +++++++++++++++++++---
 settings.c        |    4 ++--
 settings.h        |    6 ++++--
 settings_menu.c   |    3 ++-
 5 files changed, 41 insertions(+), 8 deletions(-)

Comment by Mike Schmitt (Falco98) - Sunday, 21 May 2006, 05:02 GMT+2
One more tiny little bugfix (it would prevent a setting of "Always" in "Enable Crossfade" from being saved upon reboot)...
(application/octet-stream)    track_skip_shuffle_E.patch (5.5 KiB) 
 lang/english.lang |   14 ++++++++++++++
 playback.c        |   22 +++++++++++++++++++---
 settings.c        |    4 ++--
 settings.h        |    6 ++++--
 settings_menu.c   |    3 ++-
 5 files changed, 41 insertions(+), 8 deletions(-)

Comment by Norbert Preining (norbusan) - Thursday, 22 June 2006, 11:24 GMT+2
Why do you change the #define CONFIG_BLOCK_VERSION? It seems to me that you only *changed* an entry in the config struct, not added one!?
Any ideas?
Comment by Mike Schmitt (Falco98) - Thursday, 22 June 2006, 17:37 GMT+2
Hmm. not too sure why I changed that.. IIRC I incremented it because I added a new option in the settings menu, and I thought it was supposed to be when that happened. Maybe a dev can clear this issue up for me...
Comment by Norbert Preining (norbusan) - Thursday, 22 June 2006, 18:35 GMT+2
AFAIU (and this is not a lot) you have to change it when you add an etnry only. But anyway, maybe a dev can comment.
Comment by Dave Chapman (linuxstb) - Thursday, 22 June 2006, 18:49 GMT+2
Falco98 was correct in incrementing the CONFIG_BLOCK_VERSION - he changed the definition of the crossfade option from using 2 bits to using 3 bits, which means all the settings subsequent to that option are no longer in the same place in the config block.

The only time you can change the settings struct and not increment the CONFIG_BLOCK_VERSION is if you add a new item at the end. In that case, all the existing settings stay in the same place, so there is no problem.
Comment by Norbert Preining (norbusan) - Thursday, 22 June 2006, 18:52 GMT+2
thanks for clearing this up
Comment by Mike Schmitt (Falco98) - Wednesday, 16 August 2006, 05:34 GMT+2
would anyone be willing to clean this up (i.e. synch it to recent CVS) and push it for inclusion? I would except i'm on the road 6 days a week these days, so i'm a bit rusty on the rockbox thing.
thanks!
Comment by Jon (ace214) - Wednesday, 18 October 2006, 20:30 GMT+2
tried to sync to today's cvs. am i correct in changing the CONFIG_BLOCK_VERSION to 55? (it is currently 54 in cvs). this seems to break crossfading all together- the setting for this patch exists but none of the crossfading options actually work (except for off -_-). hope somebody can take a look at it.
(application/octet-stream)    track_skip_shuffle_E_ace214_20061018.patch (5.5 KiB) 
 lang/english.lang |   14 ++++++++++++++
 playback.c        |   22 +++++++++++++++++++---
 settings.c        |    4 ++--
 settings.h        |    6 ++++--
 settings_menu.c   |    3 ++-
 5 files changed, 41 insertions(+), 8 deletions(-)

Comment by Jon (ace214) - Wednesday, 18 October 2006, 20:34 GMT+2
nevermind. it works. settings need clearing.
Comment by Jonathan Gordon (jdgordon) - Monday, 18 December 2006, 15:02 GMT+2
ok, well there doesnt seem to be any objection to adding this, so I have cleaned up the patch a bit and its attached to this.

One thing tho, is there a better way to set out the options?
<quote linuxstb on irc>Just seems to me that there are three logical on/off settings - crossfade during normal playback, crossfade during shuffled playback, and crossfade during manual skips.</quote>
which does sound reasonable to me, but neither of us use crossfade... so we could be wrong.

the other thing I wanted to check is if pcmbuf_is_crossfade_enabled() in pcmbuf.c needs changing also?
(application/octet-stream)    changes (5.4 KiB)
Comment by Mike Schmitt (Falco98) - Monday, 18 December 2006, 21:01 GMT+2
JdGordon:
you guys are right about there being 3 logical on/off settings; however, as it's been written, it only allows them to be used mutually-exclusively. separating out the options (as this original feature request asked for) would be a better implementation, but it's more code changed than I know how to do.

also: pcmbuf_is_crossfade_enabled() may have to be changed or maybe not; i'll simply say that i've been running with this patch for 6 months now and it seems to work fine. what does that variable do, exactly? you might be right that it should be changed, i wouldn't be sure one way or the other.

Loading...