[Qemu-devel] [PATCH 0/3] Audio: misc fixes for "Audio 20190821 patches"

[Qemu-devel] [PATCH 0/3] Audio: misc fixes for "Audio 20190821 patches"

[Kővágó, Zoltán]

Hi,

This series fixes two problems reported by Maxim Levitsky in relation of
my multiple audio backend patch series, and a small feature request.

Unfortunately I don't really use PulseAudio nowadays, so I haven't
tested it other than making sure it compiles and connects to pa.

Regards,
Zoltan

Kővágó, Zoltán (3):
  audio: omitting audiodev= parameter is only deprecated
  audio: paaudio: fix client name
  audio: paaudio: ability to specify stream name

 qemu-deprecated.texi | 7 +++++++
 qapi/audio.json      | 6 ++++++
 audio/audio.c        | 8 ++++----
 audio/paaudio.c      | 6 +++---
 4 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.22.0

[Qemu-devel] [PATCH 1/3] audio: omitting audiodev= parameter is only deprecated

[Kővágó, Zoltán]

Unfortunately, changes introduced in af2041ed2d "audio: audiodev=
parameters no longer optional when -audiodev present" breaks backward
compatibility.  This patch changes the error into a deprecation warning.

Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
---
 qemu-deprecated.texi | 7 +++++++
 audio/audio.c        | 8 ++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 00a4b6f350..9d74a1cfc0 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -72,6 +72,13 @@ backend settings instead of environment variables.  To ease migration to
 the new format, the ``-audiodev-help'' option can be used to convert
 the current values of the environment variables to ``-audiodev'' options.
 
+@subsection Creating sound card devices and vnc without audiodev= property (since 4.2)
+
+When not using the deprecated legacy audio config, each sound card
+should specify an @code{audiodev=} property.  Additionally, when using
+vnc, you should specify an @code{audiodev=} propery if you plan to
+transmit audio through the VNC protocol.
+
 @subsection -mon ...,control=readline,pretty=on|off (since 4.1)
 
 The @code{pretty=on|off} switch has no effect for HMP monitors, but is
diff --git a/audio/audio.c b/audio/audio.c
index 7d715332c9..e13addf922 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1412,8 +1412,9 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
         drvname = AudiodevDriver_str(dev->driver);
     } else if (!QTAILQ_EMPTY(&audio_states)) {
         if (!legacy_config) {
-            dolog("You must specify an audiodev= for the device %s\n", name);
-            exit(1);
+            dolog("Device %s: audiodev default parameter is deprecated, please "
+                  "specify audiodev=%s\n", name,
+                  QTAILQ_FIRST(&audio_states)->dev->id);
         }
         return QTAILQ_FIRST(&audio_states);
     } else {
@@ -1548,8 +1549,7 @@ CaptureVoiceOut *AUD_add_capture(
 
     if (!s) {
         if (!legacy_config) {
-            dolog("You must specify audiodev when trying to capture\n");
-            return NULL;
+            dolog("Capturing without setting an audiodev is deprecated\n");
         }
         s = audio_init(NULL, NULL);
     }
-- 
2.22.0

Re: [Qemu-devel] [PATCH 1/3] audio: omitting audiodev= parameter is only deprecated

[Maxim Levitsky]

On Mon, 2019-08-26 at 02:29 +0200, Kővágó, Zoltán wrote:
> Unfortunately, changes introduced in af2041ed2d "audio: audiodev=
> parameters no longer optional when -audiodev present" breaks backward
> compatibility.  This patch changes the error into a deprecation warning.
> 
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---
>  qemu-deprecated.texi | 7 +++++++
>  audio/audio.c        | 8 ++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 00a4b6f350..9d74a1cfc0 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -72,6 +72,13 @@ backend settings instead of environment variables.  To ease migration to
>  the new format, the ``-audiodev-help'' option can be used to convert
>  the current values of the environment variables to ``-audiodev'' options.
>  
> +@subsection Creating sound card devices and vnc without audiodev= property (since 4.2)
> +
> +When not using the deprecated legacy audio config, each sound card
> +should specify an @code{audiodev=} property.  Additionally, when using
> +vnc, you should specify an @code{audiodev=} propery if you plan to
> +transmit audio through the VNC protocol.
> +
>  @subsection -mon ...,control=readline,pretty=on|off (since 4.1)
>  
>  The @code{pretty=on|off} switch has no effect for HMP monitors, but is
> diff --git a/audio/audio.c b/audio/audio.c
> index 7d715332c9..e13addf922 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1412,8 +1412,9 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
>          drvname = AudiodevDriver_str(dev->driver);
>      } else if (!QTAILQ_EMPTY(&audio_states)) {
>          if (!legacy_config) {
> -            dolog("You must specify an audiodev= for the device %s\n", name);
> -            exit(1);
> +            dolog("Device %s: audiodev default parameter is deprecated, please "
> +                  "specify audiodev=%s\n", name,
> +                  QTAILQ_FIRST(&audio_states)->dev->id);
>          }
>          return QTAILQ_FIRST(&audio_states);
>      } else {
> @@ -1548,8 +1549,7 @@ CaptureVoiceOut *AUD_add_capture(
>  
>      if (!s) {
>          if (!legacy_config) {
> -            dolog("You must specify audiodev when trying to capture\n");
> -            return NULL;
> +            dolog("Capturing without setting an audiodev is deprecated\n");
>          }
>          s = audio_init(NULL, NULL);
>      }


This allowed me to boot th VM with single audiodev without specifying the audiodev in the device,
but on shutdown qemu crashes with heap corruption sadly.

 -audiodev pa,id=snd1,server=/run/user/103992/pulse/native
 -device ich9-intel-hda,id=sound0,msi=on
 -device hda-micro,id=sound0-codec0,bus=sound0.0,cad=0

In qemu output:
free(): invalid next size (fast)


Best regards,
	Maxim Levitsky

[Qemu-devel] [PATCH 2/3] audio: paaudio: fix client name

[Kővágó, Zoltán]

pa_context_new expects a client name, not a server socket path.

Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
---
 audio/paaudio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index bfef9acaad..777b8e4718 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -866,7 +866,7 @@ static void *qpa_conn_init(const char *server)
     }
 
     c->context = pa_context_new(pa_threaded_mainloop_get_api(c->mainloop),
-                                server);
+                                "qemu");
     if (!c->context) {
         goto fail;
     }
-- 
2.22.0

Re: [Qemu-devel] [PATCH 2/3] audio: paaudio: fix client name

[Maxim Levitsky]

On Mon, 2019-08-26 at 02:29 +0200, Kővágó, Zoltán wrote:
> pa_context_new expects a client name, not a server socket path.
> 
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---
>  audio/paaudio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/audio/paaudio.c b/audio/paaudio.c
> index bfef9acaad..777b8e4718 100644
> --- a/audio/paaudio.c
> +++ b/audio/paaudio.c
> @@ -866,7 +866,7 @@ static void *qpa_conn_init(const char *server)
>      }
>  
>      c->context = pa_context_new(pa_threaded_mainloop_get_api(c->mainloop),
> -                                server);
> +                                "qemu");
>      if (!c->context) {
>          goto fail;
>      }

Also tested, and this works.

May I suggest though to make this configurable as well, for the sake of
usability since gnome sound settings show only the client name, and it
is per each sound card.
Although on the other thing the client name is qemu.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH 2/3] audio: paaudio: fix client name

[Zoltán Kővágó]

On 2019-08-26 10:21, Maxim Levitsky wrote:
> On Mon, 2019-08-26 at 02:29 +0200, Kővágó, Zoltán wrote:
>> pa_context_new expects a client name, not a server socket path.
>>
>> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
>> ---
>>  audio/paaudio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/audio/paaudio.c b/audio/paaudio.c
>> index bfef9acaad..777b8e4718 100644
>> --- a/audio/paaudio.c
>> +++ b/audio/paaudio.c
>> @@ -866,7 +866,7 @@ static void *qpa_conn_init(const char *server)
>>      }
>>  
>>      c->context = pa_context_new(pa_threaded_mainloop_get_api(c->mainloop),
>> -                                server);
>> +                                "qemu");
>>      if (!c->context) {
>>          goto fail;
>>      }
> 
> Also tested, and this works.
> 
> May I suggest though to make this configurable as well, for the sake of
> usability since gnome sound settings show only the client name, and it
> is per each sound card.
> Although on the other thing the client name is qemu.

There is a small problem with that.  Currently we only open one
connection to pa, even with multiple -audiodevs (they will just create
different streams), which means we can only use a single client name per
qemu process.  Because of that, I wouldn't turn this into an audiodev
property.  Some other kind of global setting could work, but I'm not
sure whether it's worth it or not.

Regards,
Zoltan

> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Best regards,
> 	Maxim Levitsky
> 
> 

Re: [Qemu-devel] [PATCH 2/3] audio: paaudio: fix client name

[Maxim Levitsky]

On Mon, 2019-08-26 at 21:28 +0200, Zoltán Kővágó wrote:
> On 2019-08-26 10:21, Maxim Levitsky wrote:
> > On Mon, 2019-08-26 at 02:29 +0200, Kővágó, Zoltán wrote:
> > > pa_context_new expects a client name, not a server socket path.
> > > 
> > > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> > > ---
> > >  audio/paaudio.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/audio/paaudio.c b/audio/paaudio.c
> > > index bfef9acaad..777b8e4718 100644
> > > --- a/audio/paaudio.c
> > > +++ b/audio/paaudio.c
> > > @@ -866,7 +866,7 @@ static void *qpa_conn_init(const char *server)
> > >      }
> > >  
> > >      c->context = pa_context_new(pa_threaded_mainloop_get_api(c->mainloop),
> > > -                                server);
> > > +                                "qemu");
> > >      if (!c->context) {
> > >          goto fail;
> > >      }
> > 
> > Also tested, and this works.
> > 
> > May I suggest though to make this configurable as well, for the sake of
> > usability since gnome sound settings show only the client name, and it
> > is per each sound card.
> > Although on the other thing the client name is qemu.
> 
> There is a small problem with that.  Currently we only open one
> connection to pa, even with multiple -audiodevs (they will just create
> different streams), which means we can only use a single client name per
> qemu process.  Because of that, I wouldn't turn this into an audiodev
> property.  Some other kind of global setting could work, but I'm not
> sure whether it's worth it or not.
> 
> Regards,
> Zoltan

All right.
We could use the VM name for that though, so that at least multiple VMs
would show up as different client.


Best regards,
	Maxim Levitsky

[Qemu-devel] [PATCH 3/3] audio: paaudio: ability to specify stream name

[Kővágó, Zoltán]

This can be used to identify stream in tools like pavucontrol when one
creates multiple -audiodevs or runs multiple qemu instances.

Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
---
 qapi/audio.json | 6 ++++++
 audio/paaudio.c | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/qapi/audio.json b/qapi/audio.json
index 9fefdf5186..a433b3c9d7 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -206,6 +206,11 @@
 #
 # @name: name of the sink/source to use
 #
+# @stream-name: name of the PulseAudio stream created by qemu.  Can be
+#               used to identify the stream in PulseAudio when you
+#               create multiple PulseAudio devices or run multiple qemu
+#               instances (default "qemu", since 4.2)
+#
 # @latency: latency you want PulseAudio to achieve in microseconds
 #           (default 15000)
 #
@@ -215,6 +220,7 @@
   'base': 'AudiodevPerDirectionOptions',
   'data': {
     '*name': 'str',
+    '*stream-name': 'str',
     '*latency': 'uint32' } }
 
 ##
diff --git a/audio/paaudio.c b/audio/paaudio.c
index 777b8e4718..827f442b6e 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -562,7 +562,7 @@ static int qpa_init_out(HWVoiceOut *hw, struct audsettings *as,
 
     pa->stream = qpa_simple_new (
         c,
-        "qemu",
+        ppdo->has_stream_name ? ppdo->stream_name : "qemu",
         PA_STREAM_PLAYBACK,
         ppdo->has_name ? ppdo->name : NULL,
         &ss,
@@ -630,7 +630,7 @@ static int qpa_init_in(HWVoiceIn *hw, struct audsettings *as, void *drv_opaque)
 
     pa->stream = qpa_simple_new (
         c,
-        "qemu",
+        ppdo->has_stream_name ? ppdo->stream_name : "qemu",
         PA_STREAM_RECORD,
         ppdo->has_name ? ppdo->name : NULL,
         &ss,
-- 
2.22.0

Re: [Qemu-devel] [PATCH 3/3] audio: paaudio: ability to specify stream name

[Maxim Levitsky]

On Mon, 2019-08-26 at 02:29 +0200, Kővágó, Zoltán wrote:
> This can be used to identify stream in tools like pavucontrol when one
> creates multiple -audiodevs or runs multiple qemu instances.
> 
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---
>  qapi/audio.json | 6 ++++++
>  audio/paaudio.c | 4 ++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/audio.json b/qapi/audio.json
> index 9fefdf5186..a433b3c9d7 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -206,6 +206,11 @@
>  #
>  # @name: name of the sink/source to use
>  #
> +# @stream-name: name of the PulseAudio stream created by qemu.  Can be
> +#               used to identify the stream in PulseAudio when you
> +#               create multiple PulseAudio devices or run multiple qemu
> +#               instances (default "qemu", since 4.2)
> +#
>  # @latency: latency you want PulseAudio to achieve in microseconds
>  #           (default 15000)
>  #
> @@ -215,6 +220,7 @@
>    'base': 'AudiodevPerDirectionOptions',
>    'data': {
>      '*name': 'str',
> +    '*stream-name': 'str',
>      '*latency': 'uint32' } }
>  
>  ##
> diff --git a/audio/paaudio.c b/audio/paaudio.c
> index 777b8e4718..827f442b6e 100644
> --- a/audio/paaudio.c
> +++ b/audio/paaudio.c
> @@ -562,7 +562,7 @@ static int qpa_init_out(HWVoiceOut *hw, struct audsettings *as,
>  
>      pa->stream = qpa_simple_new (
>          c,
> -        "qemu",
> +        ppdo->has_stream_name ? ppdo->stream_name : "qemu",
>          PA_STREAM_PLAYBACK,
>          ppdo->has_name ? ppdo->name : NULL,
>          &ss,
> @@ -630,7 +630,7 @@ static int qpa_init_in(HWVoiceIn *hw, struct audsettings *as, void *drv_opaque)
>  
>      pa->stream = qpa_simple_new (
>          c,
> -        "qemu",
> +        ppdo->has_stream_name ? ppdo->stream_name : "qemu",
>          PA_STREAM_RECORD,
>          ppdo->has_name ? ppdo->name : NULL,
>          &ss,

Tested and works.
Code also seems to be OK.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky