| Message ID | a07513f1bf6123fef52ff5e7943f5704746b376b.1650874791.git.mkletzan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Remove deprecated audio features | expand |
On Mon, Apr 25, 2022 at 10:21:49AM +0200, Martin Kletzander wrote: > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > --- > ui/vnc.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index badf1d7664fe..2e7af139b030 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -4188,12 +4188,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; > - } > + if (!audiodev) { > + error_setg(errp, "Audiodev parameter for vnc required"); > + goto fail; > + } I know we deprecated not setting 'audiodev', but I'm not convinced this is the right approach. The VNC audio extension is a custom QEMU invention that few VNC clients have implemented, and even when implemented few turn it on as it is pretty awful stuttering. IMHO a better approach could be to leave audiodev optional, but stop advertizing VNC_ENCODING_AUDIO when it isn't set. IOW, current situation -vnc :1 -> enables audio capture from default backend This patch -vnc :1 -> error -vnc :1,audiodev=audio0 -> enable audio capture from 'audio0' Better: -vnc :1 -> stop advertizing VNC_ENCODING_AUDIO -vnc :1,audiodev=audio0 -> enable audio capture from 'audio0' > + > + 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"); > -- > 2.35.1 > With regards, Daniel
On 4/25/22 10:21, Martin Kletzander wrote: > @@ -4188,12 +4188,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; > - } > + if (!audiodev) { > + error_setg(errp, "Audiodev parameter for vnc required"); > + goto fail; > + } > + Wouldn't this break "-vnc :0"? You can just ignore the audio commands if vd->audio_state is NULL. Paolo
On Wed, Apr 27, 2022 at 11:32:41AM +0200, Paolo Bonzini wrote: > On 4/25/22 10:21, Martin Kletzander wrote: > > @@ -4188,12 +4188,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; > > - } > > + if (!audiodev) { > > + error_setg(errp, "Audiodev parameter for vnc required"); > > + goto fail; > > + } > > + > > Wouldn't this break "-vnc :0"? You can just ignore the audio commands if > vd->audio_state is NULL. Yep, that's wha I suggested with skipping advertizing VNC_ENCODING_AUDIO when audiodev is NULL With regards, Daniel
diff --git a/ui/vnc.c b/ui/vnc.c index badf1d7664fe..2e7af139b030 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -4188,12 +4188,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; - } + if (!audiodev) { + error_setg(errp, "Audiodev parameter for vnc required"); + goto fail; + } + + 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");
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- ui/vnc.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)