[Spice-devel] [vdagent-linux PATCH v3 1/2] audio: add functions to set volume/mute with alsa

Christophe Fergeau cfergeau at redhat.com
Wed Apr 1 02:25:12 PDT 2015


Hey,

A bunch of minor comments below

On Fri, Mar 27, 2015 at 03:09:48PM +0100, Victor Toso wrote:
> This patch includes the vdagentd-audio.[ch] files in order to
> communicate with backend audio server.
> 
> The two functions provide a way to set volume and mute in the guest
> by connecting to default mixer control in alsa which is 'Master' for
> playback and 'Capture' for record.
> ---
>  Makefile.am          |   6 +-
>  configure.ac         |   1 +
>  src/vdagentd-audio.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/vdagentd-audio.h |  27 +++++++++
>  4 files changed, 200 insertions(+), 2 deletions(-)
>  create mode 100644 src/vdagentd-audio.c
>  create mode 100644 src/vdagentd-audio.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 510f460..d789cc1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -9,11 +9,12 @@ src_spice_vdagent_LDADD = $(X_LIBS) $(SPICE_LIBS) $(GLIB2_LIBS)
>  src_spice_vdagent_SOURCES = src/vdagent.c src/vdagent-x11.c src/vdagent-x11-randr.c src/vdagent-file-xfers.c src/udscs.c
>  
>  src_spice_vdagentd_CFLAGS = $(DBUS_CFLAGS) $(LIBSYSTEMD_LOGIN_CFLAGS) \
> -  $(PCIACCESS_CFLAGS) $(SPICE_CFLAGS) $(GLIB2_CFLAGS) $(PIE_CFLAGS)
> +  $(PCIACCESS_CFLAGS) $(SPICE_CFLAGS) $(GLIB2_CFLAGS) $(PIE_CFLAGS) $(ALSA_CFLAGS)
>  src_spice_vdagentd_LDADD = $(DBUS_LIBS) $(LIBSYSTEMD_LOGIN_LIBS) \
> -  $(PCIACCESS_LIBS) $(SPICE_LIBS) $(GLIB2_LIBS) $(PIE_LDFLAGS)
> +  $(PCIACCESS_LIBS) $(SPICE_LIBS) $(GLIB2_LIBS) $(PIE_LDFLAGS) $(ALSA_LIBS)
>  src_spice_vdagentd_SOURCES = src/vdagentd.c \
>                               src/vdagentd-uinput.c \
> +                             src/vdagentd-audio.c \
>                               src/vdagentd-xorg-conf.c \
>                               src/vdagent-virtio-port.c \
>                               src/udscs.c
> @@ -37,6 +38,7 @@ noinst_HEADERS = src/glib-compat.h \
>                   src/vdagentd-proto.h \
>                   src/vdagentd-proto-strings.h \
>                   src/vdagentd-uinput.h \
> +                 src/vdagentd-audio.h \
>                   src/vdagentd-xorg-conf.h
>  
>  xdgautostartdir = $(sysconfdir)/xdg/autostart
> diff --git a/configure.ac b/configure.ac
> index 79905a8..b011358 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -79,6 +79,7 @@ AC_ARG_ENABLE([static-uinput],
>  PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.12])
>  PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
>  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.5])
> +PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
>  
>  if test "$with_session_info" = "auto" || test "$with_session_info" = "systemd"; then
>      PKG_CHECK_MODULES([LIBSYSTEMD_LOGIN],
> diff --git a/src/vdagentd-audio.c b/src/vdagentd-audio.c
> new file mode 100644
> index 0000000..c3bfe09
> --- /dev/null
> +++ b/src/vdagentd-audio.c
> @@ -0,0 +1,168 @@
> +/*  vdagentd-audio.c vdagentd audio handling code
> +
> +    Copyright 2015 Red Hat, Inc.
> +
> +    This program is free software: you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation, either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <glib.h>
> +#include <syslog.h>
> +#include <stdbool.h>
> +#include <alsa/asoundlib.h>
> +#include <alsa/mixer.h>
> +#include <alsa/error.h>
> +#include "vdagentd-audio.h"
> +
> +#define ALSA_MUTE   0
> +#define ALSA_UNMUTE 1
> +
> +static snd_mixer_elem_t *
> +alsa_get_default_mixer_by_name(snd_mixer_t **handle, const char *name)


For what it's worth, I find it a bit weird that we use the 'alsa_'
namespace in that file

> +{
> +    snd_mixer_selem_id_t *sid;
> +    int err = 0;
> +
> +    if ((err = snd_mixer_open(handle, 0)) < 0)
> +        goto fail;
> +
> +    if ((err = snd_mixer_attach(*handle, "default")) < 0)
> +        goto fail;
> +
> +    if ((err = snd_mixer_selem_register(*handle, NULL, NULL)) < 0)
> +        goto fail;
> +
> +    if ((err = snd_mixer_load(*handle)) < 0)
> +        goto fail;
> +
> +    snd_mixer_selem_id_alloca(&sid);
> +    snd_mixer_selem_id_set_index(sid, 0);
> +    snd_mixer_selem_id_set_name(sid, name);
> +    return snd_mixer_find_selem(*handle, sid);
> +
> +fail:
> +    syslog(LOG_WARNING, "%s fail: %s", __func__, snd_strerror(err));
> +    return NULL;
> +}
> +
> +static bool alsa_set_capture(uint8_t mute, uint8_t nchannels, uint16_t *volume)
> +{
> +    snd_mixer_t *handle = NULL;
> +    snd_mixer_elem_t *e;
> +    long min, max, vol;
> +    bool ret = true;
> +    int alsa_mute;
> +
> +    e = alsa_get_default_mixer_by_name (&handle, "Capture");
> +    if (e == NULL) {
> +        syslog(LOG_WARNING, "vdagentd-audio: can't get default alsa mixer");
> +        ret = false;
> +        goto end;
> +    }
> +
> +    alsa_mute = (mute) ? ALSA_MUTE : ALSA_UNMUTE;
> +    snd_mixer_selem_set_capture_switch_all(e, alsa_mute);
> +
> +    snd_mixer_selem_get_capture_volume_range(e, &min, &max);
> +    switch (nchannels) {
> +    case 1: /* MONO */
> +        vol = CLAMP(volume[0], min, max);
> +        snd_mixer_selem_set_capture_volume(e, SND_MIXER_SCHN_MONO, vol);
> +        syslog(LOG_DEBUG, "vdagentd-audio: (capture-mono) %lu (%%%0.2f)",
> +               vol, (float) (100*vol/max));

I think 100.0*vol/max would achieve the same as the (float) cast, and
would do a float division rather than an int one, giving you something
more accurate. Why %%%0.2f and not %0.2f%% ?

> +        break;
> +    case 2: /* LEFT-RIGHT */
> +        vol = CLAMP(volume[0], min, max);
> +        snd_mixer_selem_set_capture_volume(e, SND_MIXER_SCHN_FRONT_LEFT, vol);
> +        syslog(LOG_DEBUG, "vdagentd-audio: (capture-left) %lu (%%%0.2f)",
> +               vol, (float) (100*vol/max));
> +        vol = CLAMP(volume[1], min, max);
> +        snd_mixer_selem_set_capture_volume(e, SND_MIXER_SCHN_FRONT_RIGHT, vol);
> +        syslog(LOG_DEBUG, "vdagentd-audio: (capture-right) %lu (%%%0.2f)",
> +               vol, (float) (100*vol/max));
> +        break;
> +    default:
> +        syslog(LOG_WARNING, "vdagentd-audio: number of channels not supported");

I'd put the number of channels we got in the log.

Patch looks good, even without any changes.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150401/af775fd7/attachment.sig>


More information about the Spice-devel mailing list