On Thu, 2014-02-20 at 23:14 +0100, Peter Meerwald wrote:
> From: Parin Porecha <parinporecha at gmail.com>
>
> Example: pactl set-sink-volume "sink_name" 32000 40000
> If the number of volumes provided is different than the number of channels
> (excluding the case where a single volume is provided), an error message
> is displayed explaining why the volumes could not be set.
>
> patch proposed by Parin Porecha
> code refactoring and commit message slightly edited by Peter Meerwald
>
> Cc: Parin Porecha <parinporecha at gmail.com>
> ---
> src/utils/pactl.c | 131 +++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 80 insertions(+), 51 deletions(-)
>
> diff --git a/src/utils/pactl.c b/src/utils/pactl.c
> index 40e6689..8d5e197 100644
> --- a/src/utils/pactl.c
> +++ b/src/utils/pactl.c
> @@ -69,7 +69,7 @@ static bool short_list_format = false;
> static uint32_t module_index;
> static int32_t latency_offset;
> static bool suspend;
> -static pa_volume_t volume;
> +static pa_cvolume volume;
> static enum volume_flags {
> VOL_UINT = 0,
> VOL_PERCENT = 1,
> @@ -837,13 +837,16 @@ static void volume_relative_adjust(pa_cvolume *cv) {
> /* Relative volume change is additive in case of UINT or PERCENT
> * and multiplicative for LINEAR or DECIBEL */
> if ((volume_flags & 0x0F) == VOL_UINT || (volume_flags & 0x0F) == VOL_PERCENT) {
> - pa_volume_t v = pa_cvolume_avg(cv);
> - v = v + volume < PA_VOLUME_NORM ? PA_VOLUME_MUTED : v + volume - PA_VOLUME_NORM;
> - pa_cvolume_set(cv, 1, v);
> - }
> - if ((volume_flags & 0x0F) == VOL_LINEAR || (volume_flags & 0x0F) == VOL_DECIBEL) {
> - pa_sw_cvolume_multiply_scalar(cv, cv, volume);
> + unsigned i;
> + for (i = 0; i < cv->channels; i++) {
> + if (cv->values[i] + volume.values[i] < PA_VOLUME_NORM)
> + cv->values[i] = PA_VOLUME_MUTED;
> + else
> + cv->values[i] += volume.values[i] - PA_VOLUME_NORM;
I'd prefer this form for clarity:
cv->values[i] = cv->values[i] + volume.values[i] - PA_VOLUME_NORM;
The reason is that volume.values[i] - PA_VOLUME_NORM may underflow. That
shouldn't matter, because the underflow gets fixed by a subsequent
overflow, but having these underflows and overflows can confuse the
reader (I initially thought that this code was buggy, before I realized
that the overflow would compensate the underflow).
> + }
> }
> + if ((volume_flags & 0x0F) == VOL_LINEAR || (volume_flags & 0x0F) == VOL_DECIBEL)
> + pa_sw_cvolume_multiply(cv, cv, &volume);
> }
>
> static void unload_module_by_name_callback(pa_context *c, const pa_module_info *i, int is_last, void *userdata) {
> @@ -871,8 +874,26 @@ static void unload_module_by_name_callback(pa_context *c, const pa_module_info *
> }
> }
>
> +static void fill_volume(pa_cvolume *cv, unsigned supported) {
> + if (volume.channels == 1) {
> + pa_cvolume_set(&volume, supported, volume.values[0]);
> + volume.channels = supported;
This assignment is redundant, because pa_cvolume_set() already sets the
channels.
> + } else if (volume.channels != supported) {
> + pa_log(_("Failed to set volume: You tried to set volumes for %d channels, whereas channel/s supported = %d\n"),
> + volume.channels, supported);
Cosmetic: The continuation line should be aligned with the beginning of
the function arguments.
> + quit(1);
> + return;
> + }
> +
> + if ((volume_flags & VOL_RELATIVE) == VOL_RELATIVE)
The comparison is redundant. This is sufficient:
if (volume_flags & VOL_RELATIVE)
I know that the redundant comparison is used elsewhere in the code. If
you don't like the resulting inconsistency, make a cleanup patch that
removes the redundant comparisons everywhere.
> + volume_relative_adjust(cv);
> + else
> + *cv = volume;
> +}
> +
> static void get_sink_volume_callback(pa_context *c, const pa_sink_info *i, int is_last, void *userdata) {
> pa_cvolume cv;
> + unsigned channels_supported;
>
> if (is_last < 0) {
> pa_log(_("Failed to get sink information: %s"), pa_strerror(pa_context_errno(c)));
> @@ -886,12 +907,15 @@ static void get_sink_volume_callback(pa_context *c, const pa_sink_info *i, int i
> pa_assert(i);
>
> cv = i->volume;
> - volume_relative_adjust(&cv);
> + channels_supported = (&i->channel_map)->channels;
Simpler: channels_supported = i->channel_map.channels.
The channels_supported variable seems pretty redundant, though, so it
could be just removed.
> + fill_volume(&cv, channels_supported);
> +
> pa_operation_unref(pa_context_set_sink_volume_by_name(c, sink_name, &cv, simple_callback, NULL));
> }
>
> static void get_source_volume_callback(pa_context *c, const pa_source_info *i, int is_last, void *userdata) {
> pa_cvolume cv;
> + unsigned channels_supported;
>
> if (is_last < 0) {
> pa_log(_("Failed to get source information: %s"), pa_strerror(pa_context_errno(c)));
> @@ -905,12 +929,15 @@ static void get_source_volume_callback(pa_context *c, const pa_source_info *i, i
> pa_assert(i);
>
> cv = i->volume;
> - volume_relative_adjust(&cv);
> + channels_supported = (&i->channel_map)->channels;
See the previous note.
> + fill_volume(&cv, channels_supported);
> +
> pa_operation_unref(pa_context_set_source_volume_by_name(c, source_name, &cv, simple_callback, NULL));
> }
>
> static void get_sink_input_volume_callback(pa_context *c, const pa_sink_input_info *i, int is_last, void *userdata) {
> pa_cvolume cv;
> + unsigned channels_supported;
>
> if (is_last < 0) {
> pa_log(_("Failed to get sink input information: %s"), pa_strerror(pa_context_errno(c)));
> @@ -924,12 +951,15 @@ static void get_sink_input_volume_callback(pa_context *c, const pa_sink_input_in
> pa_assert(i);
>
> cv = i->volume;
> - volume_relative_adjust(&cv);
> + channels_supported = (&i->channel_map)->channels;
See the previous note.
> + fill_volume(&cv, channels_supported);
> +
> pa_operation_unref(pa_context_set_sink_input_volume(c, sink_input_idx, &cv, simple_callback, NULL));
> }
>
> static void get_source_output_volume_callback(pa_context *c, const pa_source_output_info *o, int is_last, void *userdata) {
> pa_cvolume cv;
> + unsigned channels_supported;
>
> if (is_last < 0) {
> pa_log(_("Failed to get source output information: %s"), pa_strerror(pa_context_errno(c)));
> @@ -943,7 +973,9 @@ static void get_source_output_volume_callback(pa_context *c, const pa_source_out
> pa_assert(o);
>
> cv = o->volume;
> - volume_relative_adjust(&cv);
> + channels_supported = (&o->channel_map)->channels;
See the previous note.
> + fill_volume(&cv, channels_supported);
> +
> pa_operation_unref(pa_context_set_source_output_volume(c, source_output_idx, &cv, simple_callback, NULL));
> }
>
> @@ -1309,43 +1341,19 @@ static void context_state_callback(pa_context *c, void *userdata) {
> break;
>
> case SET_SINK_VOLUME:
> - if ((volume_flags & VOL_RELATIVE) == VOL_RELATIVE) {
> - pa_operation_unref(pa_context_get_sink_info_by_name(c, sink_name, get_sink_volume_callback, NULL));
> - } else {
> - pa_cvolume v;
> - pa_cvolume_set(&v, 1, volume);
> - pa_operation_unref(pa_context_set_sink_volume_by_name(c, sink_name, &v, simple_callback, NULL));
> - }
> + pa_operation_unref(pa_context_get_sink_info_by_name(c, sink_name, get_sink_volume_callback, NULL));
> break;
>
> case SET_SOURCE_VOLUME:
> - if ((volume_flags & VOL_RELATIVE) == VOL_RELATIVE) {
> - pa_operation_unref(pa_context_get_source_info_by_name(c, source_name, get_source_volume_callback, NULL));
> - } else {
> - pa_cvolume v;
> - pa_cvolume_set(&v, 1, volume);
> - pa_operation_unref(pa_context_set_source_volume_by_name(c, source_name, &v, simple_callback, NULL));
> - }
> + pa_operation_unref(pa_context_get_source_info_by_name(c, source_name, get_source_volume_callback, NULL));
> break;
>
> case SET_SINK_INPUT_VOLUME:
> - if ((volume_flags & VOL_RELATIVE) == VOL_RELATIVE) {
> - pa_operation_unref(pa_context_get_sink_input_info(c, sink_input_idx, get_sink_input_volume_callback, NULL));
> - } else {
> - pa_cvolume v;
> - pa_cvolume_set(&v, 1, volume);
> - pa_operation_unref(pa_context_set_sink_input_volume(c, sink_input_idx, &v, simple_callback, NULL));
> - }
> + pa_operation_unref(pa_context_get_sink_input_info(c, sink_input_idx, get_sink_input_volume_callback, NULL));
> break;
>
> case SET_SOURCE_OUTPUT_VOLUME:
> - if ((volume_flags & VOL_RELATIVE) == VOL_RELATIVE) {
> - pa_operation_unref(pa_context_get_source_output_info(c, source_output_idx, get_source_output_volume_callback, NULL));
> - } else {
> - pa_cvolume v;
> - pa_cvolume_set(&v, 1, volume);
> - pa_operation_unref(pa_context_set_source_output_volume(c, source_output_idx, &v, simple_callback, NULL));
> - }
> + pa_operation_unref(pa_context_get_source_output_info(c, source_output_idx, get_source_output_volume_callback, NULL));
> break;
>
> case SET_SINK_FORMATS:
> @@ -1806,35 +1814,47 @@ int main(int argc, char *argv[]) {
> source_name = pa_xstrdup(argv[optind+1]);
>
> } else if (pa_streq(argv[optind], "set-sink-volume")) {
> + unsigned i;
> action = SET_SINK_VOLUME;
>
> - if (argc != optind+3) {
> + if (argc < optind+3) {
> pa_log(_("You have to specify a sink name/index and a volume"));
> goto quit;
> }
>
> sink_name = pa_xstrdup(argv[optind+1]);
>
> - if (parse_volume(argv[optind+2], &volume, &volume_flags) < 0)
> - goto quit;
> + volume.channels = argc-3;
> +
> + for (i = 0; i < volume.channels; i++) {
> + if (parse_volume(argv[optind+2+i],
> + &volume.values[i], &volume_flags) < 0)
> + goto quit;
> + }
It should be checked that volume.channels is valid. &volume.values[i]
will point to a bad location if i reaches PA_CHANNELS_MAX.
Also, the if condition doesn't need to be wrapped.
>
> } else if (pa_streq(argv[optind], "set-source-volume")) {
> + unsigned i;
> action = SET_SOURCE_VOLUME;
>
> - if (argc != optind+3) {
> + if (argc < optind+3) {
> pa_log(_("You have to specify a source name/index and a volume"));
> goto quit;
> }
>
> source_name = pa_xstrdup(argv[optind+1]);
>
> - if (parse_volume(argv[optind+2], &volume, &volume_flags) < 0)
> - goto quit;
> + volume.channels = argc-3;
> +
> + for (i = 0; i < volume.channels; i++) {
> + if (parse_volume(argv[optind+2+i], &volume.values[i], &volume_flags) < 0)
> + goto quit;
> + }
See the previous note.
>
> } else if (pa_streq(argv[optind], "set-sink-input-volume")) {
> + unsigned i;
> action = SET_SINK_INPUT_VOLUME;
>
> - if (argc != optind+3) {
> + if (argc < optind+3) {
> pa_log(_("You have to specify a sink input index and a volume"));
> goto quit;
> }
> @@ -1844,13 +1864,18 @@ int main(int argc, char *argv[]) {
> goto quit;
> }
>
> - if (parse_volume(argv[optind+2], &volume, &volume_flags) < 0)
> - goto quit;
> + volume.channels = argc-3;
> +
> + for (i = 0; i < volume.channels; i++) {
> + if (parse_volume(argv[optind+2+i], &volume.values[i], &volume_flags) < 0)
> + goto quit;
> + }
See the previous note.
>
> } else if (pa_streq(argv[optind], "set-source-output-volume")) {
> + unsigned i;
> action = SET_SOURCE_OUTPUT_VOLUME;
>
> - if (argc != optind+3) {
> + if (argc < optind+3) {
> pa_log(_("You have to specify a source output index and a volume"));
> goto quit;
> }
> @@ -1860,8 +1885,12 @@ int main(int argc, char *argv[]) {
> goto quit;
> }
>
> - if (parse_volume(argv[optind+2], &volume, &volume_flags) < 0)
> - goto quit;
> + volume.channels = argc-3;
> +
> + for (i = 0; i < volume.channels; i++) {
> + if (parse_volume(argv[optind+2+i], &volume.values[i], &volume_flags) < 0)
> + goto quit;
> + }
See the previous note.
--
Tanu
