Hello Tanu,
I've totally forgotten about the patch my now :)
let's see if Parin steps in, otherwise I'll do the clearup
thanks, p.
> 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.
>
>
--
Peter Meerwald
+43-664-2444418 (mobile)
