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

["Zoltán Kővágó" <dirty.ice.hu@gmail.com>]

On 2019-07-22 16:21, Markus Armbruster wrote:
> "Zoltán Kővágó" <dirty.ice.hu@gmail.com> writes:
> 
>> On 2019-07-16 08:23, Markus Armbruster wrote:
>>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes:
>>>
>>>> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
>>>> ---
>>>>    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.

Makes sense.  User will less likely use deprecated methods that way and 
it simplifies the documentation.

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

Yes.

> 
>>>
>>> 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
>>>