[pulseaudio-discuss] [PATCH 2/5] alsa: Take syncronized HW volume infra into use for alsa-sink

[Colin Guthrie]

Hi,

Review below:

'Twas brillig, and oku at iki.fi at 13/10/10 17:40 did gyre and gimble:
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index 1108a79..ada5da9 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c

> @@ -1651,8 +1713,15 @@ static int setup_mixer(struct userdata *u, pa_bool_t ignore_dB) {
>  
>          u->sink->get_volume = sink_get_volume_cb;
>          u->sink->set_volume = sink_set_volume_cb;
> +        u->sink->write_volume = sink_write_volume_cb;
> +
> +        if (sync_volume) {
> +            u->sink->flags |= PA_SINK_HW_VOLUME_CTRL |
> +                (u->mixer_path->has_dB ? (PA_SINK_DECIBEL_VOLUME | PA_SINK_SYNC_VOLUME) : 0);
> +            pa_log_info("Successfully enabled synchronous volume.");
> +        } else
> +            u->sink->flags |= PA_SINK_HW_VOLUME_CTRL | (u->mixer_path->has_dB ? PA_SINK_DECIBEL_VOLUME : 0);
>  
> -        u->sink->flags |= PA_SINK_HW_VOLUME_CTRL | (u->mixer_path->has_dB ? PA_SINK_DECIBEL_VOLUME : 0);
>          pa_log_info("Using hardware volume control. Hardware dB scale %s.", u->mixer_path->has_dB ? "supported" : "not supported");
>      }


I think this could be written more neatly.. e.g. more like:


u->sink->flags |= PA_SINK_HW_VOLUME_CTRL;
if (u->mixer_path->has_dB) {
  u->sink->flags |= PA_SINK_DECIBEL_VOLUME;
  if (sync_volume) {
    u->sink->flags |= PA_SINK_SYNC_VOLUME;
    pa_log_info(....);
  }
}

While it's more assignment operations, I think it's easier to read what
is being done.


That's all for this one :)

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]