diff mbox series

[PULL,07/15] audio: audiodev= parameters no longer optional when -audiodev present

Message ID 20190821084113.1840-8-kraxel@redhat.com
State New
Headers show
Series [PULL,01/15] audio: Add missing fall through comments | expand

Commit Message

Hoffmann, Gerd Aug. 21, 2019, 8:41 a.m. UTC 
From: Kővágó, Zoltán <dirty.ice.hu@gmail.com>

This means you should probably stop using -soundhw (as it doesn't allow
you to specify any options) and add the device manually with -device.
The exception is pcspk, it's currently not possible to manually add it.
To use it with audiodev, use something like this:

    -audiodev id=foo,... -global isa-pcspk.audiodev=foo -soundhw pcspk

Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
Message-id: 9072b955acffda13976bca7b61f86d7f708c9269.1566168923.git.DirtY.iCE.hu@gmail.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 audio/audio.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Maxim Levitsky Aug. 25, 2019, 9:44 a.m. UTC | #1 
On Wed, 2019-08-21 at 10:41 +0200, Gerd Hoffmann wrote:
> From: Kővágó, Zoltán <dirty.ice.hu@gmail.com>
> 
> This means you should probably stop using -soundhw (as it doesn't allow
> you to specify any options) and add the device manually with -device.
> The exception is pcspk, it's currently not possible to manually add it.
> To use it with audiodev, use something like this:
> 
>     -audiodev id=foo,... -global isa-pcspk.audiodev=foo -soundhw pcspk

Hi!

There is one corner case this breaks.
In qemu 4.1.0, there is no way to specify audiodev for a sound device, specifying it
fails with error.
So some of my machines have audiodev (which is miles better that using old env variables)
but also have sound devices without audiodev reference since this wasn't supported.


In what will be qemu 4.2, you must specify it, thus this kind of breaks backward compatibility.
Maybe we can have audiodev reference optional for a version or two?

This is just a minor itch, as otherwise the sound improvements are really good. The days
of installing that old realtek driver are finally gone :-)


Another thing I noted, that there is no way for pulseaudio audiodev to specify the 'client name',
it always shows up in pavucontrl as the socket path to the server. 
Thus if I added two PA audiodevs, I can't really distinguish between them.
The in|out.name= seems to specify the pulseaudio source/sink to connect to, which is not the same.

Best regards,
	Maxim Levitsky
=?UTF-8?B?Wm9sdMOhbiBLxZF2w6Fnw7M=?= Aug. 25, 2019, 6:05 p.m. UTC | #2 
On 2019-08-25 11:44, Maxim Levitsky wrote:
> On Wed, 2019-08-21 at 10:41 +0200, Gerd Hoffmann wrote:
>> From: Kővágó, Zoltán <dirty.ice.hu@gmail.com>
>>
>> This means you should probably stop using -soundhw (as it doesn't allow
>> you to specify any options) and add the device manually with -device.
>> The exception is pcspk, it's currently not possible to manually add it.
>> To use it with audiodev, use something like this:
>>
>>     -audiodev id=foo,... -global isa-pcspk.audiodev=foo -soundhw pcspk
> 
> Hi!

Hi,

> There is one corner case this breaks.
> In qemu 4.1.0, there is no way to specify audiodev for a sound device, specifying it
> fails with error.
> So some of my machines have audiodev (which is miles better that using old env variables)
> but also have sound devices without audiodev reference since this wasn't supported.
> 
> 
> In what will be qemu 4.2, you must specify it, thus this kind of breaks backward compatibility.
> Maybe we can have audiodev reference optional for a version or two?
> 
> This is just a minor itch, as otherwise the sound improvements are really good. The days
> of installing that old realtek driver are finally gone :-)

Hmm, this is what happens when you split a patch series.  We could
either revert this patch, or alternatively turn the error messages into
warnings about using deprecated behavior.

> Another thing I noted, that there is no way for pulseaudio audiodev to specify the 'client name',
> it always shows up in pavucontrl as the socket path to the server. 
> Thus if I added two PA audiodevs, I can't really distinguish between them.
> The in|out.name= seems to specify the pulseaudio source/sink to connect to, which is not the same.

We currently supply the constant "qemu" as a name to pa_stream_new.
While it's still not ideal, shouldn't this end up as a client name in
pulseaudio instead of a socket path?

Regards,
Zoltan
Maxim Levitsky Aug. 25, 2019, 10:15 p.m. UTC | #3 
On Sun, 2019-08-25 at 20:05 +0200, Zoltán Kővágó wrote:
> On 2019-08-25 11:44, Maxim Levitsky wrote:
> > On Wed, 2019-08-21 at 10:41 +0200, Gerd Hoffmann wrote:
> > > From: Kővágó, Zoltán <dirty.ice.hu@gmail.com>
> > > 
> > > This means you should probably stop using -soundhw (as it doesn't allow
> > > you to specify any options) and add the device manually with -device.
> > > The exception is pcspk, it's currently not possible to manually add it.
> > > To use it with audiodev, use something like this:
> > > 
> > >     -audiodev id=foo,... -global isa-pcspk.audiodev=foo -soundhw pcspk
> > 
> > Hi!
> 
> Hi,
> 
> > There is one corner case this breaks.
> > In qemu 4.1.0, there is no way to specify audiodev for a sound device, specifying it
> > fails with error.
> > So some of my machines have audiodev (which is miles better that using old env variables)
> > but also have sound devices without audiodev reference since this wasn't supported.
> > 
> > 
> > In what will be qemu 4.2, you must specify it, thus this kind of breaks backward compatibility.
> > Maybe we can have audiodev reference optional for a version or two?
> > 
> > This is just a minor itch, as otherwise the sound improvements are really good. The days
> > of installing that old realtek driver are finally gone :-)
> 
> Hmm, this is what happens when you split a patch series.  We could
> either revert this patch, or alternatively turn the error messages into
> warnings about using deprecated behavior.
Warning would be great in this case!
> 
> > Another thing I noted, that there is no way for pulseaudio audiodev to specify the 'client name',
> > it always shows up in pavucontrl as the socket path to the server. 
> > Thus if I added two PA audiodevs, I can't really distinguish between them.
> > The in|out.name= seems to specify the pulseaudio source/sink to connect to, which is not the same.
> 
> We currently supply the constant "qemu" as a name to pa_stream_new.
> While it's still not ideal, shouldn't this end up as a client name in
> pulseaudio instead of a socket path?

Actually it seems that pulseaudio has two names supplied for each stream
Maybe stream name and application name?

This is how chromium playback looks versus qemu in pavucontrol and in gnome volume control.

https://imgur.com/a/I8HZhgx

I do notice that 'qemu' now, in pavucontrol though.

Best regards,
	Maxim Levitsky

> 
> Regards,
> Zoltan
=?UTF-8?B?Wm9sdMOhbiBLxZF2w6Fnw7M=?= Aug. 25, 2019, 11:34 p.m. UTC | #4 
On 2019-08-26 00:15, Maxim Levitsky wrote:
> On Sun, 2019-08-25 at 20:05 +0200, Zoltán Kővágó wrote:
>> On 2019-08-25 11:44, Maxim Levitsky wrote:
>>> On Wed, 2019-08-21 at 10:41 +0200, Gerd Hoffmann wrote:
>>>> From: Kővágó, Zoltán <dirty.ice.hu@gmail.com>
>>>>
>>>> This means you should probably stop using -soundhw (as it doesn't allow
>>>> you to specify any options) and add the device manually with -device.
>>>> The exception is pcspk, it's currently not possible to manually add it.
>>>> To use it with audiodev, use something like this:
>>>>
>>>>     -audiodev id=foo,... -global isa-pcspk.audiodev=foo -soundhw pcspk
>>>
>>> Hi!
>>
>> Hi,
>>
>>> There is one corner case this breaks.
>>> In qemu 4.1.0, there is no way to specify audiodev for a sound device, specifying it
>>> fails with error.
>>> So some of my machines have audiodev (which is miles better that using old env variables)
>>> but also have sound devices without audiodev reference since this wasn't supported.
>>>
>>>
>>> In what will be qemu 4.2, you must specify it, thus this kind of breaks backward compatibility.
>>> Maybe we can have audiodev reference optional for a version or two?
>>>
>>> This is just a minor itch, as otherwise the sound improvements are really good. The days
>>> of installing that old realtek driver are finally gone :-)
>>
>> Hmm, this is what happens when you split a patch series.  We could
>> either revert this patch, or alternatively turn the error messages into
>> warnings about using deprecated behavior.
> Warning would be great in this case!
>>
>>> Another thing I noted, that there is no way for pulseaudio audiodev to specify the 'client name',
>>> it always shows up in pavucontrl as the socket path to the server. 
>>> Thus if I added two PA audiodevs, I can't really distinguish between them.
>>> The in|out.name= seems to specify the pulseaudio source/sink to connect to, which is not the same.
>>
>> We currently supply the constant "qemu" as a name to pa_stream_new.
>> While it's still not ideal, shouldn't this end up as a client name in
>> pulseaudio instead of a socket path?
> 
> Actually it seems that pulseaudio has two names supplied for each stream
> Maybe stream name and application name?
> 
> This is how chromium playback looks versus qemu in pavucontrol and in gnome volume control.
> 
> https://imgur.com/a/I8HZhgx
> 
> I do notice that 'qemu' now, in pavucontrol though.

I see.  We currently pass the server socket to pa_context_new instead of
the client name.  I'll prepare a patch soon, thanks for the report!

Regards,
Zoltan
diff mbox series

Patch

diff --git a/audio/audio.c b/audio/audio.c
index 17ef4f498fcd..c99e4ddea4c3 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -101,6 +101,8 @@  const struct mixeng_volume nominal_volume = {
 #endif
 };
 
+static bool legacy_config = true;
+
 #ifdef AUDIO_IS_FLAWLESS_AND_NO_CHECKS_ARE_REQURIED
 #error No its not
 #else
@@ -1394,7 +1396,7 @@  static AudiodevListEntry *audiodev_find(
  * if dev == NULL => legacy implicit initialization, return the already created
  *   state or create a new one
  */
-static AudioState *audio_init(Audiodev *dev)
+static AudioState *audio_init(Audiodev *dev, const char *name)
 {
     static bool atexit_registered;
     size_t i;
@@ -1408,12 +1410,13 @@  static AudioState *audio_init(Audiodev *dev)
 
     if (dev) {
         /* -audiodev option */
+        legacy_config = false;
         drvname = AudiodevDriver_str(dev->driver);
     } else if (!QTAILQ_EMPTY(&audio_states)) {
-        /*
-         * todo: check for -audiodev once we have normal audiodev selection
-         * support
-         */
+        if (!legacy_config) {
+            dolog("You must specify an audiodev= for the device %s\n", name);
+            exit(1);
+        }
         return QTAILQ_FIRST(&audio_states);
     } else {
         /* legacy implicit initialization */
@@ -1520,7 +1523,7 @@  void audio_free_audiodev_list(AudiodevListHead *head)
 void AUD_register_card (const char *name, QEMUSoundCard *card)
 {
     if (!card->state) {
-        card->state = audio_init(NULL);
+        card->state = audio_init(NULL, name);
     }
 
     card->name = g_strdup (name);
@@ -1546,8 +1549,11 @@  CaptureVoiceOut *AUD_add_capture(
     struct capture_callback *cb;
 
     if (!s) {
-        /* todo: remove when we have normal audiodev selection support */
-        s = audio_init(NULL);
+        if (!legacy_config) {
+            dolog("You must specify audiodev when trying to capture\n");
+            return NULL;
+        }
+        s = audio_init(NULL, NULL);
     }
 
     if (audio_validate_settings (as)) {
@@ -1778,7 +1784,7 @@  void audio_init_audiodevs(void)
     AudiodevListEntry *e;
 
     QSIMPLEQ_FOREACH(e, &audiodevs, next) {
-        audio_init(e->dev);
+        audio_init(e->dev, NULL);
     }
 }