Thanks for the review. I agree with your review comments and will start working on a v2 shortly. // David On 2015-10-27 06:27, Tanu Kaskinen wrote: > On Fri, 2015-09-18 at 14:47 +0200, David Henningsson wrote: >> The gnome/unity-control-center UIs have a master volume slider, and >> three sub-sliders: balance, fade, and subwoofer. Balance and fade >> use PA's set_balance and set_fade APIs accordingly, but the subwoofer >> slider sometimes does unintuitive things. >> >> In order to make that slider behave better, let's add a LFE balance >> API that these volume control UIs can use instead. With this API, >> the UI can balance between "no subwoofer" and "only subwoofer" with >> "equal balance" in the middle, which would make it more consistent >> with the behaviour of the other sliders. >> >> BugLink: https://bugzilla.gnome.org/show_bug.cgi?id=753847 >> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com> >> --- >> src/map-file | 3 +++ >> src/pulse/channelmap.c | 13 +++++++++++++ >> src/pulse/channelmap.h | 5 +++++ >> src/pulse/volume.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> src/pulse/volume.h | 18 ++++++++++++++++++ >> 5 files changed, 79 insertions(+) >> >> diff --git a/src/map-file b/src/map-file >> index 5159829..93a62b8 100644 >> --- a/src/map-file >> +++ b/src/map-file >> @@ -7,6 +7,7 @@ pa_bytes_snprint; >> pa_bytes_to_usec; >> pa_channel_map_can_balance; >> pa_channel_map_can_fade; >> +pa_channel_map_can_lfe_balance; >> pa_channel_map_compatible; >> pa_channel_map_equal; >> pa_channel_map_has_position; >> @@ -127,6 +128,7 @@ pa_cvolume_dec; >> pa_cvolume_equal; >> pa_cvolume_get_balance; >> pa_cvolume_get_fade; >> +pa_cvolume_get_lfe_balance; >> pa_cvolume_get_position; >> pa_cvolume_inc; >> pa_cvolume_inc_clamp; >> @@ -142,6 +144,7 @@ pa_cvolume_scale_mask; >> pa_cvolume_set; >> pa_cvolume_set_balance; >> pa_cvolume_set_fade; >> +pa_cvolume_set_lfe_balance; >> pa_cvolume_set_position; >> pa_cvolume_snprint; >> pa_cvolume_snprint_verbose; >> diff --git a/src/pulse/channelmap.c b/src/pulse/channelmap.c >> index c342ef6..ab343bc 100644 >> --- a/src/pulse/channelmap.c >> +++ b/src/pulse/channelmap.c >> @@ -684,6 +684,19 @@ int pa_channel_map_can_fade(const pa_channel_map *map) { >> (PA_CHANNEL_POSITION_MASK_REAR & m); >> } >> >> +int pa_channel_map_can_lfe_balance(const pa_channel_map *map) { >> + pa_channel_position_mask_t l, m; >> + >> + pa_assert(map); >> + pa_return_val_if_fail(pa_channel_map_valid(map), 0); >> + >> + m = pa_channel_map_mask(map); >> + l = PA_CHANNEL_POSITION_MASK(PA_CHANNEL_POSITION_LFE); >> + >> + return >> + (l & m) && ((~l) & m); >> +} > > I think aux channels shouldn't be considered. Possibly mono (a.k.a. > unspecified) channels should be excluded too (it's quite possible that > mono channels never coexist with lfe channels, though, so this doesn't > matter much). > >> + >> const char* pa_channel_map_to_name(const pa_channel_map *map) { >> pa_bitset_t in_map[PA_BITSET_ELEMENTS(PA_CHANNEL_POSITION_MAX)]; >> unsigned c; >> diff --git a/src/pulse/channelmap.h b/src/pulse/channelmap.h >> index 30904ef..6eabe20 100644 >> --- a/src/pulse/channelmap.h >> +++ b/src/pulse/channelmap.h >> @@ -338,6 +338,11 @@ int pa_channel_map_can_balance(const pa_channel_map *map) PA_GCC_PURE; >> * there are front/rear channels available. \since 0.9.15 */ >> int pa_channel_map_can_fade(const pa_channel_map *map) PA_GCC_PURE; >> >> +/** Returns non-zero if it makes sense to apply a volume 'lfe balance' >> + * (i.e.\ 'balance' between LFE and non-LFE channels) with this mapping, >> + * i.e.\ if there are LFE and non-LFE channels available. \since 8.0 */ >> +int pa_channel_map_can_lfe_balance(const pa_channel_map *map) PA_GCC_PURE; >> + >> /** Tries to find a well-known channel mapping name for this channel >> * mapping, i.e.\ "stereo", "surround-71" and so on. If the channel >> * mapping is unknown NULL will be returned. This name can be parsed >> diff --git a/src/pulse/volume.c b/src/pulse/volume.c >> index 2b34ded..91906be 100644 >> --- a/src/pulse/volume.c >> +++ b/src/pulse/volume.c >> @@ -552,6 +552,10 @@ static bool on_center(pa_channel_position_t p) { >> return !!(PA_CHANNEL_POSITION_MASK(p) & PA_CHANNEL_POSITION_MASK_CENTER); >> } >> >> +static bool on_hfe(pa_channel_position_t p) { >> + return !(p == PA_CHANNEL_POSITION_LFE); >> +} > > The aux/mono question applies here too. > >> + >> static bool on_lfe(pa_channel_position_t p) { >> return p == PA_CHANNEL_POSITION_LFE; >> } >> @@ -829,6 +833,42 @@ pa_cvolume* pa_cvolume_set_fade(pa_cvolume *v, const pa_channel_map *map, float >> return set_balance(v, map, new_fade, on_rear, on_front); >> } >> >> +float pa_cvolume_get_lfe_balance(const pa_cvolume *v, const pa_channel_map *map) { >> + pa_volume_t hfe, lfe; >> + >> + pa_assert(v); >> + pa_assert(map); >> + >> + pa_return_val_if_fail(pa_cvolume_compatible_with_channel_map(v, map), 0.0f); >> + >> + if (!pa_channel_map_can_lfe_balance(map)) >> + return 0.0f; >> + >> + get_avg(map, v, &hfe, &lfe, on_hfe, on_lfe); >> + >> + if (hfe == lfe) >> + return 0.0f; >> + >> + if (hfe > lfe) >> + return -1.0f + ((float) lfe / (float) hfe); >> + else >> + return 1.0f - ((float) hfe / (float) lfe); >> +} >> + >> +pa_cvolume* pa_cvolume_set_lfe_balance(pa_cvolume *v, const pa_channel_map *map, float new_balance) { >> + pa_assert(map); >> + pa_assert(v); >> + >> + pa_return_val_if_fail(pa_cvolume_compatible_with_channel_map(v, map), NULL); >> + pa_return_val_if_fail(new_balance >= -1.0f, NULL); >> + pa_return_val_if_fail(new_balance <= 1.0f, NULL); >> + >> + if (!pa_channel_map_can_lfe_balance(map)) >> + return v; >> + >> + return set_balance(v, map, new_balance, on_hfe, on_lfe); >> +} >> + >> pa_cvolume* pa_cvolume_set_position( >> pa_cvolume *cv, >> const pa_channel_map *map, >> diff --git a/src/pulse/volume.h b/src/pulse/volume.h >> index ec777b2..d749dda 100644 >> --- a/src/pulse/volume.h >> +++ b/src/pulse/volume.h >> @@ -372,6 +372,24 @@ float pa_cvolume_get_fade(const pa_cvolume *v, const pa_channel_map *map) PA_GCC >> * pa_channel_map_can_fade(). \since 0.9.15 */ >> pa_cvolume* pa_cvolume_set_fade(pa_cvolume *v, const pa_channel_map *map, float new_fade); >> >> +/** Calculate a 'lfe balance' value for the specified volume with >> + * the specified channel map. The return value will range from >> + * -1.0f (no lfe) to +1.0f (only lfe), where 0.0f is balanced. >> + * If no value is applicable to this channel map the return value >> + * will always be 0.0f. See pa_channel_map_can_fade(). \since 8.0 */ > > Copy-paste error: > s/pa_channel_map_can_fade/pa_channel_map_can_lfe_balance/ > >> +float pa_cvolume_get_lfe_balance(const pa_cvolume *v, const pa_channel_map *map) PA_GCC_PURE; > > -- > Tanu > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
