Re: [Qemu-devel] [PATCH v2 03/14] audio: add audiodev property to vnc and wav_capture

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Kővágó, Zoltán" <address@hidden> writes:
> 
> > Signed-off-by: Kővágó, Zoltán <address@hidden>
> > ---
> >  ui/vnc.h        |  2 ++
> >  monitor/misc.c  | 12 +++++++++++-
> >  ui/vnc.c        | 15 ++++++++++++++-
> >  hmp-commands.hx | 13 ++++++++-----
> >  qemu-options.hx |  6 ++++++
> >  5 files changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/ui/vnc.h b/ui/vnc.h
> > index 2f84db3142..6f54653455 100644
> > --- a/ui/vnc.h
> > +++ b/ui/vnc.h
> > @@ -183,6 +183,8 @@ struct VncDisplay
> >  #ifdef CONFIG_VNC_SASL
> >      VncDisplaySASL sasl;
> >  #endif
> > +
> > +    AudioState *audio_state;
> >  };
> >  
> >  typedef struct VncTight {
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index e393333a0e..f97810d370 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -1148,7 +1148,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;
> > +        }
> > +    }
> 
> Note for later: if "audiodev" is specified, it must name an existing
> AudioState.
> 
> >  
> >      s = g_malloc0 (sizeof (*s));
> >  
> > @@ -1156,7 +1166,7 @@ static void hmp_wavcapture(Monitor *mon, const QDict 
> > *qdict)
> >      bits = has_bits ? bits : 16;
> >      nchannels = has_channels ? nchannels : 2;
> >  
> > -    if (wav_start_capture(NULL, 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;
> 
> Note for later: this is the only other failure mode.
> 
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 140f364dda..24f9be5b5d 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -1222,7 +1222,7 @@ static void audio_add(VncState *vs)
> >      ops.destroy = audio_capture_destroy;
> >      ops.capture = audio_capture;
> >  
> > -    vs->audio_cap = AUD_add_capture(NULL, &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");
> >      }
> > @@ -3369,6 +3369,9 @@ static QemuOptsList qemu_vnc_opts = {
> >          },{
> >              .name = "non-adaptive",
> >              .type = QEMU_OPT_BOOL,
> > +        },{
> > +            .name = "audiodev",
> > +            .type = QEMU_OPT_STRING,
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -3806,6 +3809,7 @@ void vnc_display_open(const char *id, Error **errp)
> >      const char *saslauthz;
> >      int lock_key_sync = 1;
> >      int key_delay_ms;
> > +    const char *audiodev;
> >  
> >      if (!vd) {
> >          error_setg(errp, "VNC display not active");
> > @@ -3991,6 +3995,15 @@ void vnc_display_open(const char *id, Error **errp)
> >      }
> >      vd->ledstate = 0;
> >  
> > +    audiodev = qemu_opt_get(opts, "audiodev");
> > +    if (audiodev) {
> > +        vd->audio_state = audio_state_by_name(audiodev);
> > +        if (!vd->audio_state) {
> > +            error_setg(errp, "Audiodev '%s' not found", audiodev);
> > +            goto fail;
> > +        }
> > +    }
> 
> Note for later: if "audiodev" is specified, it must name an existing
> AudioState.
> 
> I like this error message better than the one in hmp_wavcapture().  Use
> it there, too?
> 
> Move it into audio_state_by_name() by giving it an Error **errp
> parameter?  Matter of taste, up to you.
> 
> > +
> >      device_id = qemu_opt_get(opts, "display");
> >      if (device_id) {
> >          int head = qemu_opt_get_number(opts, "head", 0);
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index bfa5681dd2..fa7f009268 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -819,16 +819,19 @@ 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)",
> >          .cmd        = hmp_wavcapture,
> >      },
> >  STEXI
> > -@item wavcapture @var{filename} [@var{frequency} [@var{bits} 
> > [@var{channels}]]]
> > +@item wavcapture @var{filename} [@var{frequency} [@var{bits} 
> > [@var{channels} [@var{audiodev}]]]]
> >  @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
> > +@var{frequency} bits per sample @var{bits} and number of channels
> > +@var{channels}. When not using an -audiodev argument on command line,
> > +@var{audiodev} must be omitted, otherwise is must specify a valid
> > +audiodev.
> 
> I can see the code for "must specify a valid audiodev" in
> hmp_wavcapture().  Where is "must be omitted" checked?

Isn't that just that there wont be any named devices so you wont
be able to specify a valid one?

Dave

> Preexisting: the list "sample rate @var{frequency} bits per sample
> @var{bits} and number of channels @var{channels}" lacks a comma after
> @var{frequency}, please fix that.  I'd put one after @var{bits} as well,
> but that's a matter of taste[*]
> 
> The sentence is of the form "if not COND then A else B".  The
> less-negated form "if COND then B else A" is commonly easier to read.
> 
> Documentation says "from @var{audiodev}".  But when "not using an
> -audiodev argument on command line, +@var{audiodev} must be omitted".
> Where does it sample from then?  I figure from some default audio
> device.  Where is that default audio device explained?  I skimmed the
> -audiodev documentation in qemu-options.hx, but couldn't see it there.
> 
> Suggest to say "an -audiodev command line option" instead of "an
> -audiodev argument on command line".
> 
> Double-checking:
> 
> * -audiodev is the only way to create an audio backend.
> 
> * If the user creates no audio backend, QEMU supplies a default audio
>   backend.
> 
> Correct?
> 
> Other kinds of backends can also be created at run-time with the
> monitor.  I'm not asking you provide that for audio.  I'm just wondering
> whether it could conceivably be useful.
> 
> If yes, you might want to avoid the narrow "if using -audiodev", and
> instead say "if the default audio device is in use".
> 
> >  
> >  Defaults:
> >  @itemize @minus
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9621e934c0..a308e5f5aa 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1978,6 +1978,12 @@ can help the device and guest to keep up and not 
> > lose events in case
> >  events are arriving in bulk.  Possible causes for the latter are flaky
> >  network connections, or scripts for automated testing.
> >  
> > +@item audiodev=@var{audiodev}
> > +
> > +Use the specified @var{audiodev} when the VNC client requests audio
> > +transmission. When not using an -audiodev argument, this option must
> > +be omitted, otherwise is must be present and specify a valid audiodev.
> > +
> >  @end table
> >  ETEXI
> 
> Same as for wavcapture, basically.
> 
> 
> [*] https://en.wikipedia.org/wiki/Serial_comma
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK