Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11943 - EQ settings changed via Touchscreen setting_inc & setting_dec are not reflected in the sound output

Attached to Project: Rockbox
Opened by Adam Sugerman (sugerfunk) - Tuesday, 15 February 2011, 09:37 GMT+2
Task Type Bugs
Category Themes
Status Unconfirmed
Assigned To No-one
Player Type Android
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

EQ settings changed via Touchscreen WPS action setting_inc & setting_dec are not reflected in the sound output of the device even though they display having been changed in the menus. The changes are also not written to the main config file on disk so they do not "stick" after Rockbox has been rebooted.

example syntax to toggle EQ on/off (display DOES get toggled):
%T(148,520,54,46,setting_dec,eq enabled)
%T(148,520,54,46,&setting_inc,eq enabled)
%xl(E,eq.bmp,0,0)
%V(148,520,54,46,-)
%?St(eq enabled)<%xd(E)|>

I'm not sure if this problem is specific to Android or if the problem applies to all targets.
APK Build: Feb 4, 2011
This task depends upon

Comment by Jonathan Gordon (jdgordon) - Tuesday, 15 February 2011, 10:21 GMT+2
The first part does not surprise me at all, by design settings_apply() is not called when the touchscreen button is pressed (if it did there would be lots of unnecessary disk access and possibly itwould stop/restart playback). The fix for this is to figure out the important settings here and get them to register a callback in settings_list.c so they can be applied without settings_apply() being called.


The change not getting written to config.cfg is worrying though.
Comment by Adam Sugerman (sugerfunk) - Wednesday, 16 February 2011, 09:48 GMT+2
It makes perfect sense that Rockbox wouldn't want to do a callback for most touchscreen presses.

But what about the possibility of adding one more additional (and optional) boolean parameter to the %T tag, exclusively for the setting_inc and setting_dec actions, which tells the WPS to do a settings_apply() callback only when true?
Perhaps this could make sense from an architectural perspective? It would allow EQ settings to be changed from the WPS, prevent any unnecessary callbacks by requiring an explicit request to do so, and retain backwards compatibility for all existing WPS screens.
Comment by Adam Sugerman (sugerfunk) - Wednesday, 16 February 2011, 10:26 GMT+2
One additional note: I just spent some time looking at settings_list.c & dsp.c and it appears that there is a function called dsp_set_eq() which is already defined and ready for callback (similar to the dsp_set_crossfeed callback in settings_list.c).

It is not defined in the eq_enabled declaration (and it appears that it might have been an unintentional omission):
OFFON_SETTING(F_EQSETTING, eq_enabled, LANG_EQUALIZER_ENABLED, false, "eq enabled", NULL)

Since the function is already defined in dsp.c, which is already imported, I think it would be straightforward to change it to:
OFFON_SETTING(F_EQSETTING, eq_enabled, LANG_EQUALIZER_ENABLED, false, "eq enabled", dsp_set_eq)

This looks exactly how "crossfeed" (also a boolean toggle) functions in the settings_list.c file:
OFFON_SETTING(F_SOUNDSETTING, crossfeed, LANG_CROSSFEED, false, "crossfeed", dsp_set_crossfeed)

I have yet to get a working build environment, and I haven't been able to test it, so please let me know if I'm missing something. :-)
Comment by Jonathan Gordon (jdgordon) - Wednesday, 16 February 2011, 12:18 GMT+2
that sounds about right
Comment by Adam Sugerman (sugerfunk) - Wednesday, 16 February 2011, 19:34 GMT+2
Added a patch to change settings_list.c
   settings_list.patch (0.5 KiB) 
 settings_list_update.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comment by Adam Sugerman (sugerfunk) - Thursday, 09 February 2012, 13:04 GMT+2
Any chance this patch can be applied to the main branch? It's only a simple change from NULL to a function callback, modelled after the other lines in the settings file, so I doubt it would break any functionality. I have been hoping for a while to toggle the graphic EQ on/off from the WPS screen. Thanks!
Comment by Adam Sugerman (sugerfunk) - Friday, 10 February 2012, 16:36 GMT+2
Any chance this patch can be applied to the main branch? It's only a simple change from NULL to a function callback, modelled after the other lines in the settings file, so I doubt it would break any functionality. I have been hoping for a while to toggle the graphic EQ on/off from the WPS screen. Thanks!

Loading...