| Message ID | 8245dab2e59b99b55f3f7773fd39dc920ae48951.1563224628.git.DirtY.iCE.hu@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multiple simultaneous audio backends | expand |
"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? 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
* Markus Armbruster (armbru@redhat.com) 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? 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 / dgilbert@redhat.com / Manchester, UK
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? > > 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). > 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 >
"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. 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 >>
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 >>>
On 2019-07-28 15:42, Zoltán Kővágó wrote: > 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. Actually, now that I took a second look at the params, it might be problematic. Currently audiodev is the last parameter, so if it becomes mandatory, that will effectively mean other parameters will become mandatory too. And if I were to change the order and move audiodev first, that would break backward compatibility during the deprecation period. > >> >>>> >>>> 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 >>>> >
"Zoltán Kővágó" <dirty.ice.hu@gmail.com> writes: > On 2019-07-28 15:42, Zoltán Kővágó wrote: >> 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: [...] >>>>>> 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. > > Actually, now that I took a second look at the params, it might be > problematic. Currently audiodev is the last parameter, so if it > becomes mandatory, that will effectively mean other parameters will > become mandatory too. And if I were to change the order and move > audiodev first, that would break backward compatibility during the > deprecation period. HMP is not a stable interface. We can break compatibility there when we have a reason. In this case, we can either break it right away (insert audiodev before the other optional parameters), or later (insert it last, move it when it becomes mandatory). Matter of taste. I'd break it right away.
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; + } + } 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; 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; + } + } + 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. 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
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(-)