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

[Markus Armbruster]

"Zoltán Kővágó" <address@hidden> writes:

> On 2019-07-16 08:23, Markus Armbruster 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?
>
> It's not checked right now, but if the user specifies audiodev, it
> must be a valid audiodev id.  So if the user can guess the id (which
> is not too hard ATM, it's simply the driver's name), it will work even
> in this case.
>
>> 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.
>
> Currently there are two ways to specify audio options, the legacy ones
> using the QEMU_AUDIO_* environment variables, and the new one using
> -audiodev arguments.  The two formats cannot be mixed, and eventually
> we should remove the legacy ones (IIRC my previous patch series
> already deprecated them), then we can get rid of this madness.  Maybe
> something like "When using the legacy environment variable based audio
> config, @var{audiodev} must be omitted." would be better?

What about effectively de-documenting the deprecated method?  I.e. write
as if it was already gone.  This should result in more readable
documentation.

Double-checking: will audiodev become mandatory once the deprecated
method is gone?

>>
>> 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?
>
> Not exactly a default audio backend, as it can be customized through
> environment variables, and as I previously said this is
> deprecated. When we remove the legacy config, there will be no default
> backend (like with -netdev and -device).

Thanks, I see more clearly now.

>> 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.
>
> Yes, since we can create new soundcard devices run-time, creating
> backends would make sense too.
>
>>
>> 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
>>