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