Re: [Qemu-devel] [PATCH 12/25] audio: basic support for multi backend audio

[Marc-André Lureau]

Hi

On Thu, Aug 6, 2015 at 8:28 PM, Kővágó, Zoltán <address@hidden> wrote:
> Audio functions no longer access glob_audio_state, instead they get an
> AudioState as a parameter.  This is required in order to support
> multiple backends.
>
> glob_audio_state is also gone, and replaced with a tailq so we can store
> more than one states.
>
> Signed-off-by: Kővágó, Zoltán <address@hidden>
> ---
>  audio/audio.c          | 95 
> +++++++++++++++++++++++++++++++++++++-------------
>  audio/audio.h          | 12 +++++--
>  audio/audio_int.h      |  2 ++
>  audio/audio_template.h |  2 +-
>  audio/wavcapture.c     |  6 ++--
>  hmp-commands.hx        | 11 +++---
>  monitor.c              | 12 ++++++-
>  qemu-options.hx        |  5 +++
>  ui/vnc.c               | 15 +++++++-
>  ui/vnc.h               |  2 ++
>  vl.c                   |  3 +-
>  11 files changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 05b24dc..10b9871 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -55,7 +55,8 @@ struct audio_driver *drvtab[] = {
>      NULL
>  };
>
> -static AudioState glob_audio_state;
> +static QTAILQ_HEAD(AudioStateHead, AudioState) audio_states =
> +    QTAILQ_HEAD_INITIALIZER(audio_states);
>
>  const struct mixeng_volume nominal_volume = {
>      .mute = 0,
> @@ -1193,11 +1194,14 @@ static void audio_run_capture (AudioState *s)
>
>  void audio_run (const char *msg)
>  {
> -    AudioState *s = &glob_audio_state;
> +    AudioState *s;
> +
> +    QTAILQ_FOREACH(s, &audio_states, list) {
> +        audio_run_out (s);
> +        audio_run_in (s);
> +        audio_run_capture (s);
> +    }
>
> -    audio_run_out (s);
> -    audio_run_in (s);
> -    audio_run_capture (s);
>  #ifdef DEBUG_POLL
>      {
>          static double prevtime;
> @@ -1252,9 +1256,8 @@ static void audio_vm_change_state_handler (void 
> *opaque, int running,
>      audio_reset_timer (s);
>  }
>
> -static void audio_atexit (void)
> +static void free_audio_state(AudioState *s)
>  {
> -    AudioState *s = &glob_audio_state;
>      HWVoiceOut *hwo = NULL;
>      HWVoiceIn *hwi = NULL;
>
> @@ -1288,6 +1291,16 @@ static void audio_atexit (void)
>      }
>
>      qapi_free_Audiodev(s->dev);
> +    g_free(s);
> +}
> +
> +static void audio_atexit(void)
> +{
> +    while (!QTAILQ_EMPTY(&audio_states)) {
> +        AudioState *s = QTAILQ_FIRST(&audio_states);
> +        QTAILQ_REMOVE(&audio_states, s, list);
> +        free_audio_state(s);
> +    }
>  }
>
>  static const VMStateDescription vmstate_audio = {
> @@ -1300,26 +1313,25 @@ static const VMStateDescription vmstate_audio = {
>  };
>
>  static Audiodev *parse_option(QemuOpts *opts, Error **errp);
> -static int audio_init(Audiodev *dev)
> +static AudioState *audio_init(Audiodev *dev)
>  {
> +    static bool atexit_registered;
>      size_t i;
>      int done = 0;
>      const char *drvname = NULL;
>      VMChangeStateEntry *e;
> -    AudioState *s = &glob_audio_state;
> +    AudioState *s;
>      QemuOptsList *list = NULL; /* silence gcc warning about uninitialized
>                                  * variable */
>
> -    if (s->drv) {
> -        if (dev) {
> -            dolog("Cannot create more than one audio backend, sorry\n");
> -            qapi_free_Audiodev(dev);
> -        }
> -        return -1;
> -    }
> -
> +    /* if we have dev, this function was called because of an -audiodev
> +     * argument => initialize a new state with it
> +     * if dev == NULL => legacy implicit initialization, return the already
> +     * created state or create a new one */

I think you could place this comment above the function.

>      if (dev) {
>          drvname = AudiodevDriver_lookup[dev->kind];
> +    } else if (!QTAILQ_EMPTY(&audio_states)) {
> +        return QTAILQ_FIRST(&audio_states);
>      } else {
>          audio_handle_legacy_opts();
>          list = qemu_find_opts("audiodev");
> @@ -1328,12 +1340,18 @@ static int audio_init(Audiodev *dev)
>              exit(1);
>          }
>      }
> +
> +    s = g_malloc0(sizeof(AudioState));
>      s->dev = dev;
>
>      QLIST_INIT (&s->hw_head_out);
>      QLIST_INIT (&s->hw_head_in);
>      QLIST_INIT (&s->cap_head);
> -    atexit (audio_atexit);
> +    if (!atexit_registered) {
> +        atexit(audio_atexit);
> +        atexit_registered = true;
> +    }
> +    QTAILQ_INSERT_TAIL(&audio_states, s, list);
>
>      s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
>      if (!s->ts) {
> @@ -1414,15 +1432,18 @@ static int audio_init(Audiodev *dev)
>
>      QLIST_INIT (&s->card_head);
>      vmstate_register (NULL, 0, &vmstate_audio, s);
> -    return 0;
> +    return s;
>  }
>
>  void AUD_register_card (const char *name, QEMUSoundCard *card)
>  {
> -    audio_init(NULL);
> +    if (!card->state) {
> +        card->state = audio_init(NULL);
> +    }
> +
>      card->name = g_strdup (name);
>      memset (&card->entries, 0, sizeof (card->entries));
> -    QLIST_INSERT_HEAD (&glob_audio_state.card_head, card, entries);
> +    QLIST_INSERT_HEAD (&card->state->card_head, card, entries);
>  }
>
>  void AUD_remove_card (QEMUSoundCard *card)
> @@ -1432,16 +1453,20 @@ void AUD_remove_card (QEMUSoundCard *card)
>  }
>
>
> -CaptureVoiceOut *AUD_add_capture (
> +CaptureVoiceOut *AUD_add_capture(
> +    AudioState *s,
>      struct audsettings *as,
>      struct audio_capture_ops *ops,
>      void *cb_opaque
>      )
>  {
> -    AudioState *s = &glob_audio_state;
>      CaptureVoiceOut *cap;
>      struct capture_callback *cb;
>
> +    if (!s) { /* todo */

What's the todo here for?

> +        s = QTAILQ_FIRST(&audio_states);
> +    }
> +
>      if (audio_validate_settings (as)) {
>          dolog ("Invalid settings were passed when trying to add capture\n");
>          audio_print_settings (as);
> @@ -1677,7 +1702,7 @@ static int each_option(void *opaque, QemuOpts *opts, 
> Error **errp)
>      if (!dev) {
>          return -1;
>      }
> -    return audio_init(dev);
> +    return audio_init(dev) ? 0 : -1;
>  }
>
>  void audio_set_options(void)
> @@ -1743,3 +1768,25 @@ int audio_buffer_bytes(AudiodevPerDirectionOptions 
> *pdo,
>      return audio_buffer_samples(pdo, as, def_usecs) *
>          audioformat_bytes_per_sample(as->fmt);
>  }
> +
> +AudioState *audio_state_by_name(const char *name)
> +{
> +    AudioState *s;
> +    QTAILQ_FOREACH(s, &audio_states, list) {
> +        assert(s->dev);
> +        if (strcmp(name, s->dev->id) == 0) {
> +            return s;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +const char *audio_get_id(QEMUSoundCard *card)
> +{
> +    if (card->state) {
> +        assert(card->state->dev);
> +        return card->state->dev->id;
> +    } else {
> +        return "";
> +    }
> +}
> diff --git a/audio/audio.h b/audio/audio.h
> index 177a673..0085a07 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -80,8 +80,10 @@ typedef struct SWVoiceOut SWVoiceOut;
>  typedef struct CaptureVoiceOut CaptureVoiceOut;
>  typedef struct SWVoiceIn SWVoiceIn;
>
> +typedef struct AudioState AudioState;
>  typedef struct QEMUSoundCard {
>      char *name;
> +    AudioState *state;
>      QLIST_ENTRY (QEMUSoundCard) entries;
>  } QEMUSoundCard;
>
> @@ -96,7 +98,8 @@ void AUD_log (const char *cap, const char *fmt, ...) 
> GCC_FMT_ATTR(2, 3);
>
>  void AUD_register_card (const char *name, QEMUSoundCard *card);
>  void AUD_remove_card (QEMUSoundCard *card);
> -CaptureVoiceOut *AUD_add_capture (
> +CaptureVoiceOut *AUD_add_capture(
> +    AudioState *s,
>      struct audsettings *as,
>      struct audio_capture_ops *ops,
>      void *opaque
> @@ -164,11 +167,14 @@ static inline void *advance (void *p, int incr)
>  #define audio_MAX(a, b) ((a)<(b)?(b):(a))
>  #endif
>
> -int wav_start_capture (CaptureState *s, const char *path, int freq,
> -                       int bits, int nchannels);
> +int wav_start_capture(AudioState *state, CaptureState *s, const char *path,
> +                      int freq, int bits, int nchannels);
>
>  void audio_set_options(void);
>  void audio_handle_legacy_opts(void);
>  void audio_legacy_help(void);
>
> +AudioState *audio_state_by_name(const char *name);
> +const char *audio_get_id(QEMUSoundCard *card);
> +
>  #endif  /* audio.h */
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 101081b..1d81658 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -189,6 +189,8 @@ struct AudioState {
>      int nb_hw_voices_in;
>      int vm_running;
>      int64_t period_ticks;
> +
> +    QTAILQ_ENTRY(AudioState) list;
>  };
>
>  extern struct audio_driver no_audio_driver;
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 455e50d..5a3dc90 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -399,7 +399,7 @@ SW *glue (AUD_open_, TYPE) (
>      struct audsettings *as
>      )
>  {
> -    AudioState *s = &glob_audio_state;
> +    AudioState *s = card->state;
>      AudiodevPerDirectionOptions *pdo = s->dev->TYPE;
>
>      if (audio_bug (AUDIO_FUNC, !card || !name || !callback_fn || !as)) {
> diff --git a/audio/wavcapture.c b/audio/wavcapture.c
> index 798acdd..8b8d3d0 100644
> --- a/audio/wavcapture.c
> +++ b/audio/wavcapture.c
> @@ -104,8 +104,8 @@ static struct capture_ops wav_capture_ops = {
>      .info = wav_capture_info
>  };
>
> -int wav_start_capture (CaptureState *s, const char *path, int freq,
> -                       int bits, int nchannels)
> +int wav_start_capture(AudioState *state, CaptureState *s, const char *path,
> +                      int freq, int bits, int nchannels)
>  {
>      Monitor *mon = cur_mon;
>      WAVState *wav;
> @@ -172,7 +172,7 @@ int wav_start_capture (CaptureState *s, const char *path, 
> int freq,
>          goto error_free;
>      }
>
> -    cap = AUD_add_capture (&as, &ops, wav);
> +    cap = AUD_add_capture(state, &as, &ops, wav);
>      if (!cap) {
>          monitor_printf (mon, "Failed to add audio capture\n");
>          goto error_free;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d3b7932..9b49dc3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -741,16 +741,17 @@ ETEXI
>
>      {
>          .name       = "wavcapture",
> -        .args_type  = "path:F,freq:i?,bits:i?,nchannels:i?",
> -        .params     = "path [frequency [bits [channels]]]",
> +        .args_type  = "path:F,freq:i?,bits:i?,nchannels:i?,audiodev:s?",
> +        .params     = "path [frequency [bits [channels [audiodev]]]]",
>          .help       = "capture audio to a wave file (default frequency=44100 
> bits=16 channels=2)",
>          .mhandler.cmd = hmp_wavcapture,
>      },
>  STEXI
> address@hidden wavcapture @var{filename} address@hidden address@hidden 
> address@hidden
> address@hidden wavcapture @var{filename} address@hidden address@hidden 
> address@hidden address@hidden
>  @findex wavcapture
> -Capture audio into @var{filename}. Using sample rate @var{frequency}
> -bits per sample @var{bits} and number of channels @var{channels}.
> +Capture audio into @var{filename} from @var{audiodev}. Using sample rate
> address@hidden bits per sample @var{bits} and number of channels
> address@hidden
>
>  Defaults:
>  @itemize @minus
> diff --git a/monitor.c b/monitor.c
> index a40138b..f527753 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1944,7 +1944,17 @@ static void hmp_wavcapture(Monitor *mon, const QDict 
> *qdict)
>      int bits = qdict_get_try_int(qdict, "bits", -1);
>      int has_channels = qdict_haskey(qdict, "nchannels");
>      int nchannels = qdict_get_try_int(qdict, "nchannels", -1);
> +    const char *audiodev = qdict_get_try_str(qdict, "audiodev");
>      CaptureState *s;
> +    AudioState *as = NULL;
> +
> +    if (audiodev) {
> +        as = audio_state_by_name(audiodev);
> +        if (!as) {
> +            monitor_printf(mon, "Invalid audiodev specified\n");
> +            return;
> +        }
> +    }
>
>      s = g_malloc0 (sizeof (*s));
>
> @@ -1952,7 +1962,7 @@ static void hmp_wavcapture(Monitor *mon, const QDict 
> *qdict)
>      bits = has_bits ? bits : 16;
>      nchannels = has_channels ? nchannels : 2;
>
> -    if (wav_start_capture (s, path, freq, bits, nchannels)) {
> +    if (wav_start_capture(as, s, path, freq, bits, nchannels)) {
>          monitor_printf(mon, "Failed to add wave capture\n");
>          g_free (s);
>          return;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index efd57e6..60a3563 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1538,6 +1538,11 @@ everybody else.  'ignore' completely ignores the 
> shared flag and
>  allows everybody connect unconditionally.  Doesn't conform to the rfb
>  spec but is traditional QEMU behavior.
>
> address@hidden address@hidden
> +
> +Use the specified @var{audiodev} when the VNC client requests audio
> +transmission.

That doesn't make much sense to me, vnc (or spice) should be connected
to the guest device, not to the audiodev. Same for for the wavcapture.

> +
>  @end table
>  ETEXI
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index febbad6..feee4ef 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1193,7 +1193,7 @@ static void audio_add(VncState *vs)
>      ops.destroy = audio_capture_destroy;
>      ops.capture = audio_capture;
>
> -    vs->audio_cap = AUD_add_capture(&vs->as, &ops, vs);
> +    vs->audio_cap = AUD_add_capture(vs->vd->audio_state, &vs->as, &ops, vs);
>      if (!vs->audio_cap) {
>          error_report("Failed to add audio capture");
>      }
> @@ -3295,6 +3295,9 @@ static QemuOptsList qemu_vnc_opts = {
>          },{
>              .name = "non-adaptive",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "audiodev",
> +            .type = QEMU_OPT_STRING,
>          },
>          { /* end of list */ }
>      },
> @@ -3455,6 +3458,7 @@ void vnc_display_open(const char *id, Error **errp)
>      int acl = 0;
>  #endif
>      int lock_key_sync = 1;
> +    const char *audiodev;
>
>      if (!vs) {
>          error_setg(errp, "VNC display not active");
> @@ -3637,6 +3641,15 @@ void vnc_display_open(const char *id, Error **errp)
>  #endif
>      vs->lock_key_sync = lock_key_sync;
>
> +    audiodev = qemu_opt_get(opts, "audiodev");
> +    if (audiodev) {
> +        vs->audio_state = audio_state_by_name(audiodev);
> +        if (!vs->audio_state) {
> +            error_setg(errp, "Audiodev '%s' not found", audiodev);
> +            goto fail;
> +        }
> +    }
> +
>      device_id = qemu_opt_get(opts, "display");
>      if (device_id) {
>          DeviceState *dev;
> diff --git a/ui/vnc.h b/ui/vnc.h
> index 814d720..f73bb94 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -187,6 +187,8 @@ struct VncDisplay
>  #ifdef CONFIG_VNC_SASL
>      VncDisplaySASL sasl;
>  #endif
> +
> +    AudioState *audio_state;
>  };
>
>  typedef struct VncTight {
> diff --git a/vl.c b/vl.c
> index f1aa1df..30bc8be 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4315,6 +4315,8 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>
> +    audio_set_options();
> +
>      machine_opts = qemu_get_machine_opts();
>      if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>                           NULL)) {
> @@ -4518,7 +4520,6 @@ int main(int argc, char **argv, char **envp)
>
>      realtime_init();
>
> -    audio_set_options();
>      audio_init();
>
>      cpu_synchronize_all_post_init();
> --
> 2.4.5
>
>



-- 
Marc-André Lureau