* [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches"
@ 2019-08-26 19:59 Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 1/4] audio: fix invalid malloc size in audio_create_pdos Kővágó, Zoltán
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Kővágó, Zoltán @ 2019-08-26 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Maxim Levitsky
Hi,
This is the second iteration of my "random fixes" patchset. Compared to
v1, this has a new patch, fixing the memory error caused by an invalid
malloc.
Regards,
Zoltan
Kővágó, Zoltán (4):
audio: fix invalid malloc size in audio_create_pdos
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 | 10 +++++-----
audio/paaudio.c | 6 +++---
4 files changed, 21 insertions(+), 8 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] audio: fix invalid malloc size in audio_create_pdos
2019-08-26 19:59 [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Kővágó, Zoltán
@ 2019-08-26 19:59 ` Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 2/4] audio: omitting audiodev= parameter is only deprecated Kővágó, Zoltán
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Kővágó, Zoltán @ 2019-08-26 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Maxim Levitsky
The code used sizeof(AudiodevAlsaPerDirectionOptions) instead of the
appropriate per direction options for the audio backend. If the size of
the actual audiodev's per direction options are larger than alsa's, it
could cause a buffer overflow.
However, alsa has three fields in per direction options: a string, an
uint32 and a bool. Oss has the same fields, coreaudio has a single
uint32, paaudio has a string and an uint32, all other backends only use
the common options, so currently no per direction options struct should
be larger than alsa's.
Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
---
audio/audio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/audio/audio.c b/audio/audio.c
index 7d715332c9..ae335dbebb 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1685,7 +1685,7 @@ void audio_create_pdos(Audiodev *dev)
} \
if (!dev->u.driver.has_out) { \
dev->u.driver.out = g_malloc0( \
- sizeof(AudiodevAlsaPerDirectionOptions)); \
+ sizeof(Audiodev##pdo_name##PerDirectionOptions)); \
dev->u.driver.has_out = true; \
} \
break
--
2.22.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] audio: omitting audiodev= parameter is only deprecated
2019-08-26 19:59 [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 1/4] audio: fix invalid malloc size in audio_create_pdos Kővágó, Zoltán
@ 2019-08-26 19:59 ` Kővágó, Zoltán
2019-08-28 11:53 ` Gerd Hoffmann
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name Kővágó, Zoltán
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Kővágó, Zoltán @ 2019-08-26 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: reviewer:Incompatible changes, Gerd Hoffmann, Maxim Levitsky
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 ae335dbebb..e99fcd0694 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name
2019-08-26 19:59 [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 1/4] audio: fix invalid malloc size in audio_create_pdos Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 2/4] audio: omitting audiodev= parameter is only deprecated Kővágó, Zoltán
@ 2019-08-26 19:59 ` Kővágó, Zoltán
2019-08-27 5:37 ` Gerd Hoffmann
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name Kővágó, Zoltán
2019-08-26 21:43 ` [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Maxim Levitsky
4 siblings, 1 reply; 19+ messages in thread
From: Kővágó, Zoltán @ 2019-08-26 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Maxim Levitsky
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>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-26 19:59 [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Kővágó, Zoltán
` (2 preceding siblings ...)
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name Kővágó, Zoltán
@ 2019-08-26 19:59 ` Kővágó, Zoltán
2019-08-27 5:42 ` Gerd Hoffmann
2019-08-26 21:43 ` [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Maxim Levitsky
4 siblings, 1 reply; 19+ messages in thread
From: Kővágó, Zoltán @ 2019-08-26 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, Gerd Hoffmann, Maxim Levitsky
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>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches"
2019-08-26 19:59 [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Kővágó, Zoltán
` (3 preceding siblings ...)
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name Kővágó, Zoltán
@ 2019-08-26 21:43 ` Maxim Levitsky
4 siblings, 0 replies; 19+ messages in thread
From: Maxim Levitsky @ 2019-08-26 21:43 UTC (permalink / raw)
To: Kővágó, Zoltán, qemu-devel
On Mon, 2019-08-26 at 21:59 +0200, Kővágó, Zoltán wrote:
> Hi,
>
> This is the second iteration of my "random fixes" patchset. Compared to
> v1, this has a new patch, fixing the memory error caused by an invalid
> malloc.
>
> Regards,
> Zoltan
>
> Kővágó, Zoltán (4):
> audio: fix invalid malloc size in audio_create_pdos
> 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 | 10 +++++-----
> audio/paaudio.c | 6 +++---
> 4 files changed, 21 insertions(+), 8 deletions(-)
>
With that patch series everything works as expected.
No more heap corruption when I don't use audiodev.
Thanks!
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name Kővágó, Zoltán
@ 2019-08-27 5:37 ` Gerd Hoffmann
2019-08-27 11:26 ` Maxim Levitsky
0 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2019-08-27 5:37 UTC (permalink / raw)
To: Kővágó, Zoltán; +Cc: qemu-devel, Maxim Levitsky
On Mon, Aug 26, 2019 at 09:59:03PM +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>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.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");
qemu_get_vm_name() would be a better default (returns the name set by
the user using "qemu -name $whatever", can be NULL if unset).
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name Kővágó, Zoltán
@ 2019-08-27 5:42 ` Gerd Hoffmann
2019-08-27 22:43 ` Zoltán Kővágó
0 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2019-08-27 5:42 UTC (permalink / raw)
To: Kővágó, Zoltán
Cc: Markus Armbruster, qemu-devel, Maxim Levitsky
On Mon, Aug 26, 2019 at 09:59:04PM +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.
Hmm, can we create an useful name automatically, without yet another
config option?
Useful choices could be the device name (usb-audio, ...) or the device
id (whatever -device id=xxx was specified on the command line).
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name
2019-08-27 5:37 ` Gerd Hoffmann
@ 2019-08-27 11:26 ` Maxim Levitsky
0 siblings, 0 replies; 19+ messages in thread
From: Maxim Levitsky @ 2019-08-27 11:26 UTC (permalink / raw)
To: Gerd Hoffmann, Kővágó, Zoltán; +Cc: qemu-devel
On Tue, 2019-08-27 at 07:37 +0200, Gerd Hoffmann wrote:
> On Mon, Aug 26, 2019 at 09:59:03PM +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>
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.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");
>
> qemu_get_vm_name() would be a better default (returns the name set by
> the user using "qemu -name $whatever", can be NULL if unset).
That is exactly what I was thinking about this too.
Best regards,
Maxim Levitsky
>
> cheers,
> Gerd
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-27 5:42 ` Gerd Hoffmann
@ 2019-08-27 22:43 ` Zoltán Kővágó
2019-08-28 7:33 ` Gerd Hoffmann
2019-08-28 9:53 ` Daniel P. Berrangé
0 siblings, 2 replies; 19+ messages in thread
From: Zoltán Kővágó @ 2019-08-27 22:43 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Markus Armbruster, qemu-devel, Maxim Levitsky
On 2019-08-27 07:42, Gerd Hoffmann wrote:
> On Mon, Aug 26, 2019 at 09:59:04PM +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.
>
> Hmm, can we create an useful name automatically, without yet another
> config option?
>
> Useful choices could be the device name (usb-audio, ...) or the device
> id (whatever -device id=xxx was specified on the command line).
I'm afraid this is not going to work with the current architecture: due
to mixeng even if you have multiple devices, they'll be mixed to a
single stream and the audio backend will only see this one mixed stream.
As a workaround we could do something like concat all device names or
ids, but I don't like that idea.
Alternatively we could use the id of the audiodev instead, and no more
problems with mixeng. However, with mixeng off (implemented in my next
patch series) suddenly soundcards will have suddenly end up as different
streams. (This can be worked around by creating multiple audiodevs,
like what you have to use now to get multiple streams from pa, so this
is probably a smaller problem.)
Currently I'm leaning for the audiodev's id option, unless someone
proposes something better.
Regards,
Zoltan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-27 22:43 ` Zoltán Kővágó
@ 2019-08-28 7:33 ` Gerd Hoffmann
2019-08-28 9:12 ` Maxim Levitsky
2019-08-28 9:53 ` Daniel P. Berrangé
1 sibling, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2019-08-28 7:33 UTC (permalink / raw)
To: Zoltán Kővágó
Cc: Markus Armbruster, qemu-devel, Maxim Levitsky
> > Hmm, can we create an useful name automatically, without yet another
> > config option?
> >
> > Useful choices could be the device name (usb-audio, ...) or the device
> > id (whatever -device id=xxx was specified on the command line).
>
> I'm afraid this is not going to work with the current architecture: due
> to mixeng even if you have multiple devices, they'll be mixed to a
> single stream and the audio backend will only see this one mixed stream.
> As a workaround we could do something like concat all device names or
> ids, but I don't like that idea.
>
> Alternatively we could use the id of the audiodev instead, and no more
> problems with mixeng. However, with mixeng off (implemented in my next
> patch series) suddenly soundcards will have suddenly end up as different
> streams. (This can be worked around by creating multiple audiodevs,
> like what you have to use now to get multiple streams from pa, so this
> is probably a smaller problem.)
>
> Currently I'm leaning for the audiodev's id option, unless someone
> proposes something better.
Hmm, maybe just wait and see if we really need that?
If we use the vm name for the pa connection I suspect most use cases are
covered ...
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-28 7:33 ` Gerd Hoffmann
@ 2019-08-28 9:12 ` Maxim Levitsky
0 siblings, 0 replies; 19+ messages in thread
From: Maxim Levitsky @ 2019-08-28 9:12 UTC (permalink / raw)
To: Gerd Hoffmann, Zoltán Kővágó
Cc: qemu-devel, Markus Armbruster
On Wed, 2019-08-28 at 09:33 +0200, Gerd Hoffmann wrote:
> > > Hmm, can we create an useful name automatically, without yet another
> > > config option?
> > >
> > > Useful choices could be the device name (usb-audio, ...) or the device
> > > id (whatever -device id=xxx was specified on the command line).
> >
> > I'm afraid this is not going to work with the current architecture: due
> > to mixeng even if you have multiple devices, they'll be mixed to a
> > single stream and the audio backend will only see this one mixed stream.
> > As a workaround we could do something like concat all device names or
> > ids, but I don't like that idea.
> >
> > Alternatively we could use the id of the audiodev instead, and no more
> > problems with mixeng. However, with mixeng off (implemented in my next
> > patch series) suddenly soundcards will have suddenly end up as different
> > streams. (This can be worked around by creating multiple audiodevs,
> > like what you have to use now to get multiple streams from pa, so this
> > is probably a smaller problem.)
> >
> > Currently I'm leaning for the audiodev's id option, unless someone
> > proposes something better.
>
> Hmm, maybe just wait and see if we really need that?
>
> If we use the vm name for the pa connection I suspect most use cases are
> covered ...
My personal opinion would be, to use vm name for the connection (will cover
use case of distinguishing between VMs)
and using audiodev derived name for stream names, which will at least
partially cover the use case of having more that one guestn's soundcard,
and assigning each to different host sound card dynamically,
that is using pavucontrol, which is what I wanted to do in first place.
This is also should be the easiest to do probably.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-27 22:43 ` Zoltán Kővágó
2019-08-28 7:33 ` Gerd Hoffmann
@ 2019-08-28 9:53 ` Daniel P. Berrangé
2019-08-28 10:14 ` Maxim Levitsky
1 sibling, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2019-08-28 9:53 UTC (permalink / raw)
To: Zoltán Kővágó
Cc: qemu-devel, Maxim Levitsky, Gerd Hoffmann, Markus Armbruster
On Wed, Aug 28, 2019 at 12:43:49AM +0200, Zoltán Kővágó wrote:
> On 2019-08-27 07:42, Gerd Hoffmann wrote:
> > On Mon, Aug 26, 2019 at 09:59:04PM +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.
> >
> > Hmm, can we create an useful name automatically, without yet another
> > config option?
> >
> > Useful choices could be the device name (usb-audio, ...) or the device
> > id (whatever -device id=xxx was specified on the command line).
>
> I'm afraid this is not going to work with the current architecture: due
> to mixeng even if you have multiple devices, they'll be mixed to a
> single stream and the audio backend will only see this one mixed stream.
> As a workaround we could do something like concat all device names or
> ids, but I don't like that idea.
>
> Alternatively we could use the id of the audiodev instead, and no more
> problems with mixeng. However, with mixeng off (implemented in my next
> patch series) suddenly soundcards will have suddenly end up as different
> streams. (This can be worked around by creating multiple audiodevs,
> like what you have to use now to get multiple streams from pa, so this
> is probably a smaller problem.)
>
> Currently I'm leaning for the audiodev's id option, unless someone
> proposes something better.
Using the audiodev id is not a good idea. If you have multiple QEMU's
on your host, it is highly likely that libvirt will have assigned
the same audiodev id to all of them. Using the vm name would be ok,
but only if you assume that each gust only has a single audio device.
Using a combination of vm name + audidev id is going to be unique
per host, but not especially friendly as a user visible name. It
would be ok as a default, but I'd think we should let the mgmt app
specify stream name explicitly, so that something user friendly
can be set.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-28 9:53 ` Daniel P. Berrangé
@ 2019-08-28 10:14 ` Maxim Levitsky
2019-08-28 10:26 ` Daniel P. Berrangé
0 siblings, 1 reply; 19+ messages in thread
From: Maxim Levitsky @ 2019-08-28 10:14 UTC (permalink / raw)
To: Daniel P. Berrangé, Zoltán Kővágó
Cc: qemu-devel, Gerd Hoffmann, Markus Armbruster
On Wed, 2019-08-28 at 10:53 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 28, 2019 at 12:43:49AM +0200, Zoltán Kővágó wrote:
> > On 2019-08-27 07:42, Gerd Hoffmann wrote:
> > > On Mon, Aug 26, 2019 at 09:59:04PM +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.
> > >
> > > Hmm, can we create an useful name automatically, without yet another
> > > config option?
> > >
> > > Useful choices could be the device name (usb-audio, ...) or the device
> > > id (whatever -device id=xxx was specified on the command line).
> >
> > I'm afraid this is not going to work with the current architecture: due
> > to mixeng even if you have multiple devices, they'll be mixed to a
> > single stream and the audio backend will only see this one mixed stream.
> > As a workaround we could do something like concat all device names or
> > ids, but I don't like that idea.
> >
> > Alternatively we could use the id of the audiodev instead, and no more
> > problems with mixeng. However, with mixeng off (implemented in my next
> > patch series) suddenly soundcards will have suddenly end up as different
> > streams. (This can be worked around by creating multiple audiodevs,
> > like what you have to use now to get multiple streams from pa, so this
> > is probably a smaller problem.)
> >
> > Currently I'm leaning for the audiodev's id option, unless someone
> > proposes something better.
>
> Using the audiodev id is not a good idea. If you have multiple QEMU's
> on your host, it is highly likely that libvirt will have assigned
> the same audiodev id to all of them. Using the vm name would be ok,
> but only if you assume that each gust only has a single audio device.
>
> Using a combination of vm name + audidev id is going to be unique
> per host, but not especially friendly as a user visible name. It
> would be ok as a default, but I'd think we should let the mgmt app
> specify stream name explicitly, so that something user friendly
> can be set.
No, no!
It seems that pulseaudio has a name for each connection, and a name for each
steam within that connection.
The suggestion is that we use the VM name for the connection,
(which will be unique per VM usually, at least the user can make it be so)
and then use the audiodev id for each stream. Of course for multiple VMs,
the audiodev ids will be the same, but this is all right since you can
always distinguish them that the streams come from different VMs.
Also note that this thing is cosmetic from the correctness point of view,
that is pulse-audio internally has no problem with duplicate IDs.
The thing is useful mostly for tweaking the output streams in the pavucontrol,
where the names will allow you to easily know which steam is which.
Speaking of this, there is another minor feature missing, which is volume level
feedback from pavucontrol to the guest. That means that if you change the volume of the
stream in pavucontrol, the guest will not be aware of this, and still think that the
volume is the same.
But for 'tweaking' I was mostly thinking about assigning the input/output streams to
different sound devices, which pavucontrol can do and does work very well.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-28 10:14 ` Maxim Levitsky
@ 2019-08-28 10:26 ` Daniel P. Berrangé
2019-08-28 10:39 ` Maxim Levitsky
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2019-08-28 10:26 UTC (permalink / raw)
To: Maxim Levitsky
Cc: qemu-devel, Markus Armbruster, Gerd Hoffmann,
Zoltán Kővágó
On Wed, Aug 28, 2019 at 01:14:03PM +0300, Maxim Levitsky wrote:
> On Wed, 2019-08-28 at 10:53 +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 28, 2019 at 12:43:49AM +0200, Zoltán Kővágó wrote:
> > > On 2019-08-27 07:42, Gerd Hoffmann wrote:
> > > > On Mon, Aug 26, 2019 at 09:59:04PM +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.
> > > >
> > > > Hmm, can we create an useful name automatically, without yet another
> > > > config option?
> > > >
> > > > Useful choices could be the device name (usb-audio, ...) or the device
> > > > id (whatever -device id=xxx was specified on the command line).
> > >
> > > I'm afraid this is not going to work with the current architecture: due
> > > to mixeng even if you have multiple devices, they'll be mixed to a
> > > single stream and the audio backend will only see this one mixed stream.
> > > As a workaround we could do something like concat all device names or
> > > ids, but I don't like that idea.
> > >
> > > Alternatively we could use the id of the audiodev instead, and no more
> > > problems with mixeng. However, with mixeng off (implemented in my next
> > > patch series) suddenly soundcards will have suddenly end up as different
> > > streams. (This can be worked around by creating multiple audiodevs,
> > > like what you have to use now to get multiple streams from pa, so this
> > > is probably a smaller problem.)
> > >
> > > Currently I'm leaning for the audiodev's id option, unless someone
> > > proposes something better.
> >
> > Using the audiodev id is not a good idea. If you have multiple QEMU's
> > on your host, it is highly likely that libvirt will have assigned
> > the same audiodev id to all of them. Using the vm name would be ok,
> > but only if you assume that each gust only has a single audio device.
> >
> > Using a combination of vm name + audidev id is going to be unique
> > per host, but not especially friendly as a user visible name. It
> > would be ok as a default, but I'd think we should let the mgmt app
> > specify stream name explicitly, so that something user friendly
> > can be set.
> No, no!
> It seems that pulseaudio has a name for each connection, and a name for each
> steam within that connection.
>
> The suggestion is that we use the VM name for the connection,
> (which will be unique per VM usually, at least the user can make it be so)
> and then use the audiodev id for each stream. Of course for multiple VMs,
> the audiodev ids will be the same, but this is all right since you can
> always distinguish them that the streams come from different VMs.
Ok, if I'm reading the code correctly, it seems we do take care to
re-use a single connection to PA for all audiodevs we create. So a
VMname is fine for the connection.
> Also note that this thing is cosmetic from the correctness point of view,
> that is pulse-audio internally has no problem with duplicate IDs.
>
> The thing is useful mostly for tweaking the output streams in the pavucontrol,
> where the names will allow you to easily know which steam is which.
Yep, I wasn't really concerned about internals - from the user POV being
able to accurately distinguish streams in pavucontrol is very important
though, so we should ensure that's possible. If we use 'id' for the
stream as a default though, we should still allow an override, as 'id'
values are not really intended as end user visible data. If a guest
has multiple devices I'd expect to be able to give them names that are
meaningful to me as a user, not something libvirt auto-generates for
its own machine oriented use.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-28 10:26 ` Daniel P. Berrangé
@ 2019-08-28 10:39 ` Maxim Levitsky
2019-09-09 23:42 ` Zoltán Kővágó
0 siblings, 1 reply; 19+ messages in thread
From: Maxim Levitsky @ 2019-08-28 10:39 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Markus Armbruster, Gerd Hoffmann,
Zoltán Kővágó
On Wed, 2019-08-28 at 11:26 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 28, 2019 at 01:14:03PM +0300, Maxim Levitsky wrote:
> > On Wed, 2019-08-28 at 10:53 +0100, Daniel P. Berrangé wrote:
> > > On Wed, Aug 28, 2019 at 12:43:49AM +0200, Zoltán Kővágó wrote:
> > > > On 2019-08-27 07:42, Gerd Hoffmann wrote:
> > > > > On Mon, Aug 26, 2019 at 09:59:04PM +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.
> > > > >
> > > > > Hmm, can we create an useful name automatically, without yet another
> > > > > config option?
> > > > >
> > > > > Useful choices could be the device name (usb-audio, ...) or the device
> > > > > id (whatever -device id=xxx was specified on the command line).
> > > >
> > > > I'm afraid this is not going to work with the current architecture: due
> > > > to mixeng even if you have multiple devices, they'll be mixed to a
> > > > single stream and the audio backend will only see this one mixed stream.
> > > > As a workaround we could do something like concat all device names or
> > > > ids, but I don't like that idea.
> > > >
> > > > Alternatively we could use the id of the audiodev instead, and no more
> > > > problems with mixeng. However, with mixeng off (implemented in my next
> > > > patch series) suddenly soundcards will have suddenly end up as different
> > > > streams. (This can be worked around by creating multiple audiodevs,
> > > > like what you have to use now to get multiple streams from pa, so this
> > > > is probably a smaller problem.)
> > > >
> > > > Currently I'm leaning for the audiodev's id option, unless someone
> > > > proposes something better.
> > >
> > > Using the audiodev id is not a good idea. If you have multiple QEMU's
> > > on your host, it is highly likely that libvirt will have assigned
> > > the same audiodev id to all of them. Using the vm name would be ok,
> > > but only if you assume that each gust only has a single audio device.
> > >
> > > Using a combination of vm name + audidev id is going to be unique
> > > per host, but not especially friendly as a user visible name. It
> > > would be ok as a default, but I'd think we should let the mgmt app
> > > specify stream name explicitly, so that something user friendly
> > > can be set.
> >
> > No, no!
> > It seems that pulseaudio has a name for each connection, and a name for each
> > steam within that connection.
> >
> > The suggestion is that we use the VM name for the connection,
> > (which will be unique per VM usually, at least the user can make it be so)
> > and then use the audiodev id for each stream. Of course for multiple VMs,
> > the audiodev ids will be the same, but this is all right since you can
> > always distinguish them that the streams come from different VMs.
>
> Ok, if I'm reading the code correctly, it seems we do take care to
> re-use a single connection to PA for all audiodevs we create. So a
> VMname is fine for the connection.
>
> > Also note that this thing is cosmetic from the correctness point of view,
> > that is pulse-audio internally has no problem with duplicate IDs.
> >
> > The thing is useful mostly for tweaking the output streams in the pavucontrol,
> > where the names will allow you to easily know which steam is which.
>
> Yep, I wasn't really concerned about internals - from the user POV being
> able to accurately distinguish streams in pavucontrol is very important
> though, so we should ensure that's possible. If we use 'id' for the
> stream as a default though, we should still allow an override, as 'id'
> values are not really intended as end user visible data. If a guest
> has multiple devices I'd expect to be able to give them names that are
> meaningful to me as a user, not something libvirt auto-generates for
> its own machine oriented use.
I have absolutely nothing against user specified override!
Just that if the idea is shot down, lets at least have device id instead.
For the reference this is how currently the sound streams are shown,
without any patches applied
https://imgur.com/a/I8HZhgx
Gnome sound panel only shows application names,
but pavucontrol shows both the application name and stream name.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] audio: omitting audiodev= parameter is only deprecated
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 2/4] audio: omitting audiodev= parameter is only deprecated Kővágó, Zoltán
@ 2019-08-28 11:53 ` Gerd Hoffmann
0 siblings, 0 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2019-08-28 11:53 UTC (permalink / raw)
To: Kővágó, Zoltán
Cc: reviewer:Incompatible changes, qemu-devel, Maxim Levitsky
On Mon, Aug 26, 2019 at 09:59:02PM +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.
Prepared a pull request for 1+2 to get them merged quickly while the pa
naming discussion is ongoing still.
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-08-28 10:39 ` Maxim Levitsky
@ 2019-09-09 23:42 ` Zoltán Kővágó
2019-09-10 7:00 ` Gerd Hoffmann
0 siblings, 1 reply; 19+ messages in thread
From: Zoltán Kővágó @ 2019-09-09 23:42 UTC (permalink / raw)
To: Maxim Levitsky, Daniel P. Berrangé
Cc: qemu-devel, Gerd Hoffmann, Markus Armbruster
On 2019-08-28 12:39, Maxim Levitsky wrote:
> On Wed, 2019-08-28 at 11:26 +0100, Daniel P. Berrangé wrote:
>> On Wed, Aug 28, 2019 at 01:14:03PM +0300, Maxim Levitsky wrote:
>>> On Wed, 2019-08-28 at 10:53 +0100, Daniel P. Berrangé wrote:
>>>> On Wed, Aug 28, 2019 at 12:43:49AM +0200, Zoltán Kővágó wrote:
>>>>> On 2019-08-27 07:42, Gerd Hoffmann wrote:
>>>>>> On Mon, Aug 26, 2019 at 09:59:04PM +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.
>>>>>>
>>>>>> Hmm, can we create an useful name automatically, without yet another
>>>>>> config option?
>>>>>>
>>>>>> Useful choices could be the device name (usb-audio, ...) or the device
>>>>>> id (whatever -device id=xxx was specified on the command line).
>>>>>
>>>>> I'm afraid this is not going to work with the current architecture: due
>>>>> to mixeng even if you have multiple devices, they'll be mixed to a
>>>>> single stream and the audio backend will only see this one mixed stream.
>>>>> As a workaround we could do something like concat all device names or
>>>>> ids, but I don't like that idea.
>>>>>
>>>>> Alternatively we could use the id of the audiodev instead, and no more
>>>>> problems with mixeng. However, with mixeng off (implemented in my next
>>>>> patch series) suddenly soundcards will have suddenly end up as different
>>>>> streams. (This can be worked around by creating multiple audiodevs,
>>>>> like what you have to use now to get multiple streams from pa, so this
>>>>> is probably a smaller problem.)
>>>>>
>>>>> Currently I'm leaning for the audiodev's id option, unless someone
>>>>> proposes something better.
>>>>
>>>> Using the audiodev id is not a good idea. If you have multiple QEMU's
>>>> on your host, it is highly likely that libvirt will have assigned
>>>> the same audiodev id to all of them. Using the vm name would be ok,
>>>> but only if you assume that each gust only has a single audio device.
>>>>
>>>> Using a combination of vm name + audidev id is going to be unique
>>>> per host, but not especially friendly as a user visible name. It
>>>> would be ok as a default, but I'd think we should let the mgmt app
>>>> specify stream name explicitly, so that something user friendly
>>>> can be set.
>>>
>>> No, no!
>>> It seems that pulseaudio has a name for each connection, and a name for each
>>> steam within that connection.
>>>
>>> The suggestion is that we use the VM name for the connection,
>>> (which will be unique per VM usually, at least the user can make it be so)
>>> and then use the audiodev id for each stream. Of course for multiple VMs,
>>> the audiodev ids will be the same, but this is all right since you can
>>> always distinguish them that the streams come from different VMs.
>>
>> Ok, if I'm reading the code correctly, it seems we do take care to
>> re-use a single connection to PA for all audiodevs we create. So a
>> VMname is fine for the connection.
>>
>>> Also note that this thing is cosmetic from the correctness point of view,
>>> that is pulse-audio internally has no problem with duplicate IDs.
>>>
>>> The thing is useful mostly for tweaking the output streams in the pavucontrol,
>>> where the names will allow you to easily know which steam is which.
>>
>> Yep, I wasn't really concerned about internals - from the user POV being
>> able to accurately distinguish streams in pavucontrol is very important
>> though, so we should ensure that's possible. If we use 'id' for the
>> stream as a default though, we should still allow an override, as 'id'
>> values are not really intended as end user visible data. If a guest
>> has multiple devices I'd expect to be able to give them names that are
>> meaningful to me as a user, not something libvirt auto-generates for
>> its own machine oriented use.
>
> I have absolutely nothing against user specified override!
> Just that if the idea is shot down, lets at least have device id instead.
>
>
> For the reference this is how currently the sound streams are shown,
> without any patches applied
> https://imgur.com/a/I8HZhgx
>
> Gnome sound panel only shows application names,
> but pavucontrol shows both the application name and stream name.
>
> Best regards,
> Maxim Levitsky
>
Ping.
If I understand the situation correctly, the current consensus is:
* use VM name for PA server connection
* audiodev id is a good default for PA stream name
What is not clear whether we need a separate qapi option for stream
name, or just always use the audiodev id. I don't use pulseaudio or
libvirt, so I can't really comment about this issue.
Ideally I'd like to create a new patch with this change and a fix for
the coverity issue reported in [1].
Regards,
Zoltan
[1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg01543.html
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name
2019-09-09 23:42 ` Zoltán Kővágó
@ 2019-09-10 7:00 ` Gerd Hoffmann
0 siblings, 0 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2019-09-10 7:00 UTC (permalink / raw)
To: Zoltán Kővágó
Cc: qemu-devel, Daniel P. Berrangé, Markus Armbruster, Maxim Levitsky
> Ping.
>
> If I understand the situation correctly, the current consensus is:
>
> * use VM name for PA server connection
Yes (most important one IMHO).
> * audiodev id is a good default for PA stream name
Yes.
> What is not clear whether we need a separate qapi option for stream name, or
> just always use the audiodev id. I don't use pulseaudio or libvirt, so I
> can't really comment about this issue.
No clear consensus. I'd suggest to just ignore that for now,
or make it a separate patch.
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-09-10 7:02 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 19:59 [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 1/4] audio: fix invalid malloc size in audio_create_pdos Kővágó, Zoltán
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 2/4] audio: omitting audiodev= parameter is only deprecated Kővágó, Zoltán
2019-08-28 11:53 ` Gerd Hoffmann
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 3/4] audio: paaudio: fix client name Kővágó, Zoltán
2019-08-27 5:37 ` Gerd Hoffmann
2019-08-27 11:26 ` Maxim Levitsky
2019-08-26 19:59 ` [Qemu-devel] [PATCH v2 4/4] audio: paaudio: ability to specify stream name Kővágó, Zoltán
2019-08-27 5:42 ` Gerd Hoffmann
2019-08-27 22:43 ` Zoltán Kővágó
2019-08-28 7:33 ` Gerd Hoffmann
2019-08-28 9:12 ` Maxim Levitsky
2019-08-28 9:53 ` Daniel P. Berrangé
2019-08-28 10:14 ` Maxim Levitsky
2019-08-28 10:26 ` Daniel P. Berrangé
2019-08-28 10:39 ` Maxim Levitsky
2019-09-09 23:42 ` Zoltán Kővágó
2019-09-10 7:00 ` Gerd Hoffmann
2019-08-26 21:43 ` [Qemu-devel] [PATCH v2 0/4] Audio: misc fixes for "Audio 20190821 patches" Maxim Levitsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).