This patch provides a default value to connect
jack ports when the user don't specify connect-ports.

Buglink: https://bugs.launchpad.net/qemu/+bug/1908832

Signed-off-by: José Pekkarinen <koalinux@gmail.com>
---
 audio/jackaudio.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 3031c4e29b..0a87d5e23a 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -369,14 +369,23 @@ static size_t qjack_read(HWVoiceIn *hw, void *buf, size_t len)
 
 static void qjack_client_connect_ports(QJackClient *c)
 {
-    if (!c->connect_ports || !c->opt->connect_ports) {
+    if (!c->connect_ports) {
         return;
     }
 
     c->connect_ports = false;
     const char **ports;
-    ports = jack_get_ports(c->client, c->opt->connect_ports, NULL,
-        c->out ? JackPortIsInput : JackPortIsOutput);
+    if (c->out) {
+        ports = jack_get_ports(c->client,
+            c->opt->connect_ports ? c->opt->connect_ports
+                : "system:playback_.*",
+            NULL, JackPortIsInput);
+    } else {
+        ports = jack_get_ports(c->client,
+            c->opt->connect_ports ? c->opt->connect_ports
+                : "system:capture_.*",
+            NULL, JackPortIsOutput);
+    }
 
     if (!ports) {
         return;
-- 
2.26.2

On Mittwoch, 24. Februar 2021 20:19:27 CET José Pekkarinen wrote:
> This patch provides a default value to connect
> jack ports when the user don't specify connect-ports.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1908832
> 
> Signed-off-by: José Pekkarinen <koalinux@gmail.com>
> ---
>  audio/jackaudio.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 3031c4e29b..0a87d5e23a 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -369,14 +369,23 @@ static size_t qjack_read(HWVoiceIn *hw, void *buf,
> size_t len)
> 
>  static void qjack_client_connect_ports(QJackClient *c)
>  {
> -    if (!c->connect_ports || !c->opt->connect_ports) {
> +    if (!c->connect_ports) {
>          return;
>      }
> 
>      c->connect_ports = false;
>      const char **ports;
> -    ports = jack_get_ports(c->client, c->opt->connect_ports, NULL,
> -        c->out ? JackPortIsInput : JackPortIsOutput);
> +    if (c->out) {
> +        ports = jack_get_ports(c->client,
> +            c->opt->connect_ports ? c->opt->connect_ports
> +                : "system:playback_.*",
> +            NULL, JackPortIsInput);
> +    } else {
> +        ports = jack_get_ports(c->client,
> +            c->opt->connect_ports ? c->opt->connect_ports
> +                : "system:capture_.*",
> +            NULL, JackPortIsOutput);
> +    }
> 
>      if (!ports) {
>          return;

Looks fine to me now.

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Thanks José!

Best regards,
Christian Schoenebeck

This goes against how all standard jack clients work, a new jack client 
should not auto-connect at all unless explicitly configured to as if 
there is an existing audio diagram configured (which is 99% of the time) 
it will cause unexpected/undesired behavior.

Jack is not supposed to be an 'automatic' system, it's the 
responsibility of the patch bay software to route connections.

The auto-connect feature exists to allow the jack audiodev to re-connect 
a broken connection when the jack device restarts/reconnects.

On 2021-02-25 06:39, Christian Schoenebeck wrote:
> On Mittwoch, 24. Februar 2021 20:19:27 CET José Pekkarinen wrote:
>> This patch provides a default value to connect
>> jack ports when the user don't specify connect-ports.
>> 
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1908832
>> 
>> Signed-off-by: José Pekkarinen <koalinux@gmail.com>
>> ---
>>  audio/jackaudio.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
>> index 3031c4e29b..0a87d5e23a 100644
>> --- a/audio/jackaudio.c
>> +++ b/audio/jackaudio.c
>> @@ -369,14 +369,23 @@ static size_t qjack_read(HWVoiceIn *hw, void 
>> *buf,
>> size_t len)
>> 
>>  static void qjack_client_connect_ports(QJackClient *c)
>>  {
>> -    if (!c->connect_ports || !c->opt->connect_ports) {
>> +    if (!c->connect_ports) {
>>          return;
>>      }
>> 
>>      c->connect_ports = false;
>>      const char **ports;
>> -    ports = jack_get_ports(c->client, c->opt->connect_ports, NULL,
>> -        c->out ? JackPortIsInput : JackPortIsOutput);
>> +    if (c->out) {
>> +        ports = jack_get_ports(c->client,
>> +            c->opt->connect_ports ? c->opt->connect_ports
>> +                : "system:playback_.*",
>> +            NULL, JackPortIsInput);
>> +    } else {
>> +        ports = jack_get_ports(c->client,
>> +            c->opt->connect_ports ? c->opt->connect_ports
>> +                : "system:capture_.*",
>> +            NULL, JackPortIsOutput);
>> +    }
>> 
>>      if (!ports) {
>>          return;
> 
> Looks fine to me now.
> 
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 
> Thanks José!
> 
> Best regards,
> Christian Schoenebeck

On Mittwoch, 24. Februar 2021 23:04:47 CET Geoffrey McRae wrote:
> This goes against how all standard jack clients work, a new jack client
> should not auto-connect at all unless explicitly configured to as if
> there is an existing audio diagram configured (which is 99% of the time)
> it will cause unexpected/undesired behavior.
> 
> Jack is not supposed to be an 'automatic' system, it's the
> responsibility of the patch bay software to route connections.
> 
> The auto-connect feature exists to allow the jack audiodev to re-connect
> a broken connection when the jack device restarts/reconnects.

Well, that was also my idea first, and I would agree with you in case of a 
regular music app of course, but then I thought QEMU is probably not an 
average JACK client, and it simply lowers the entry level for new users who 
probably just want to output to system out anyway.

I mean, are you piping QEMU into Ardour or something Geoffrey?

This could still be overridden by passing a bogus pattern with argument 
"connect-ports" for people who prefer the patchbay approach in the end.

So I would vote for the "make it easy for newbies" approach in this case, but 
I leave that up to you and Gerd to decide. :)

Best regards,
Christian Schoenebeck

While I get where you're coming from, those using QEMU with Jack are 
already advanced users that are used to reading technical documentation. 
Having our one client do something that is unexpected/different would 
not only confuse existing Jack users but also anyone following any 
guides/documentation on how to use a generic jack client. IMO the better 
solution here is simply better documentation, perhaps even a known 
working sample setup.

On 2021-02-25 09:33, Christian Schoenebeck wrote:
> On Mittwoch, 24. Februar 2021 23:04:47 CET Geoffrey McRae wrote:
>> This goes against how all standard jack clients work, a new jack 
>> client
>> should not auto-connect at all unless explicitly configured to as if
>> there is an existing audio diagram configured (which is 99% of the 
>> time)
>> it will cause unexpected/undesired behavior.
>> 
>> Jack is not supposed to be an 'automatic' system, it's the
>> responsibility of the patch bay software to route connections.
>> 
>> The auto-connect feature exists to allow the jack audiodev to 
>> re-connect
>> a broken connection when the jack device restarts/reconnects.
> 
> Well, that was also my idea first, and I would agree with you in case 
> of a
> regular music app of course, but then I thought QEMU is probably not an
> average JACK client, and it simply lowers the entry level for new users 
> who
> probably just want to output to system out anyway.
> 
> I mean, are you piping QEMU into Ardour or something Geoffrey?
> 
> This could still be overridden by passing a bogus pattern with argument
> "connect-ports" for people who prefer the patchbay approach in the end.

Sorry but this is a kludge to reverse another kludge, I really don't 
think this is a good way to go.

> 
> So I would vote for the "make it easy for newbies" approach in this 
> case, but
> I leave that up to you and Gerd to decide. :)
> 
> Best regards,
> Christian Schoenebeck

On Thu, 25 Feb 2021 at 00:38, Geoffrey McRae <geoff@hostfission.com> wrote:

> While I get where you're coming from, those using QEMU with Jack are
> already advanced users that are used to reading technical documentation.
> Having our one client do something that is unexpected/different would
> not only confuse existing Jack users but also anyone following any
> guides/documentation on how to use a generic jack client. IMO the better
> solution here is simply better documentation, perhaps even a known
> working sample setup.
>

    This is an interesting point, but hey, things be told in advance,
whether
the patch is accepted or not is up to you, I'll respect any decision in the
upstreaming, I'm just curious, how can a default behaviour that multiple
other applications and libraries adopt is going to confuse jack community?
For instance, I tend to work with firefox, and mumble that natively support
jack, and they perform automatic connection no patchbay extra config
required, the basics just work out of the box, while you still can do more
complex stuff if you want to use the full set of features of jack.

    On the other hand, I took some time to read to automatically configure
the connection over qjackctl patchbay, so thanks for pointing out that via.

    José.

On Wed, Feb 24, 2021 at 11:33:14PM +0100, Christian Schoenebeck wrote:
> On Mittwoch, 24. Februar 2021 23:04:47 CET Geoffrey McRae wrote:
> > This goes against how all standard jack clients work, a new jack client
> > should not auto-connect at all unless explicitly configured to as if
> > there is an existing audio diagram configured (which is 99% of the time)
> > it will cause unexpected/undesired behavior.
> > 
> > Jack is not supposed to be an 'automatic' system, it's the
> > responsibility of the patch bay software to route connections.
> > 
> > The auto-connect feature exists to allow the jack audiodev to re-connect
> > a broken connection when the jack device restarts/reconnects.
> 
> Well, that was also my idea first, and I would agree with you in case of a 
> regular music app of course, but then I thought QEMU is probably not an 
> average JACK client, and it simply lowers the entry level for new users who 
> probably just want to output to system out anyway.

Well, I guess there is more software like that, any music player for
example.  I don't think this is a good reason for qemu to have
non-standard behavior.  If you want qemu autoconnect, you can use the
connect-ports option.

Beside that I'd expect the patch bay software is able to remember the
routing configuration per application, so the setup would be a one-time
thing you don't have to re-do on every qemu launch.  Not fully sure this
is actually the case though, I'm not a regular jack user.

take care,
  Gerd

On Donnerstag, 25. Februar 2021 09:48:55 CET Gerd Hoffmann wrote:
> On Wed, Feb 24, 2021 at 11:33:14PM +0100, Christian Schoenebeck wrote:
> > On Mittwoch, 24. Februar 2021 23:04:47 CET Geoffrey McRae wrote:
> > > This goes against how all standard jack clients work, a new jack client
> > > should not auto-connect at all unless explicitly configured to as if
> > > there is an existing audio diagram configured (which is 99% of the time)
> > > it will cause unexpected/undesired behavior.
> > > 
> > > Jack is not supposed to be an 'automatic' system, it's the
> > > responsibility of the patch bay software to route connections.
> > > 
> > > The auto-connect feature exists to allow the jack audiodev to re-connect
> > > a broken connection when the jack device restarts/reconnects.
> > 
> > Well, that was also my idea first, and I would agree with you in case of a
> > regular music app of course, but then I thought QEMU is probably not an
> > average JACK client, and it simply lowers the entry level for new users
> > who
> > probably just want to output to system out anyway.
> 
> Well, I guess there is more software like that, any music player for
> example.  I don't think this is a good reason for qemu to have
> non-standard behavior.  If you want qemu autoconnect, you can use the
> connect-ports option.
> 
> Beside that I'd expect the patch bay software is able to remember the
> routing configuration per application, so the setup would be a one-time
> thing you don't have to re-do on every qemu launch.  Not fully sure this
> is actually the case though, I'm not a regular jack user.
> 
> take care,
>   Gerd

Yes, a JACK client with patchbay functionality like QJackCtl stores and 
restores individual connections. You need to start it each time for doing that 
though.

JACK clients with consumer purpose often auto connect to system ports by 
default because their users mostly use JACK just as a consumer desktop sound 
server. And I assume this applies to José as well.

Whereas JACK clients intended for musicians typically don't, because their 
common use case is to pipe audio through several music apps in their personal, 
and very custom way.

Then there are also a bunch of JACK clients with built-in support for some of 
the available session management standards. In this scenario you have another 
app, a session manager, which not only takes care about connections, but also 
actually starts all individual JACK client apps with their respective previous 
internal configurations.

Best regards,
Christian Schoenebeck

  Hi,

> JACK clients with consumer purpose often auto connect to system ports by 
> default because their users mostly use JACK just as a consumer desktop sound 
> server. And I assume this applies to José as well.

Hmm, ok.  I'd suggest to simply change the default for connect-ports
then, that'll allow the user to easily change the behavior by setting
connect-ports to something else (including the empty string to disable
autoconnect).

take care,
  Gerd

On Mittwoch, 3. März 2021 08:13:06 CET Gerd Hoffmann wrote:
>   Hi,
> 
> > JACK clients with consumer purpose often auto connect to system ports by
> > default because their users mostly use JACK just as a consumer desktop
> > sound server. And I assume this applies to José as well.
> 
> Hmm, ok.  I'd suggest to simply change the default for connect-ports
> then, that'll allow the user to easily change the behavior by setting
> connect-ports to something else (including the empty string to disable
> autoconnect).
> 
> take care,
>   Gerd

Geoffrey, any chance to make you happy as well? E.g. either reserving "none" 
as special value for "connect-ports" or an additional CL argument
"no-connect-ports" to make it appear less hackish?

Best regards,
Christian Schoenebeck

On Thu, Mar 04, 2021 at 12:24:44PM +0100, Christian Schoenebeck wrote:
> On Mittwoch, 3. März 2021 08:13:06 CET Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > JACK clients with consumer purpose often auto connect to system ports by
> > > default because their users mostly use JACK just as a consumer desktop
> > > sound server. And I assume this applies to José as well.
> > 
> > Hmm, ok.  I'd suggest to simply change the default for connect-ports
> > then, that'll allow the user to easily change the behavior by setting
> > connect-ports to something else (including the empty string to disable
> > autoconnect).
> > 
> > take care,
> >   Gerd
> 
> Geoffrey, any chance to make you happy as well? E.g. either reserving "none" 
> as special value for "connect-ports" or an additional CL argument
> "no-connect-ports" to make it appear less hackish?

Adding special semantics for existing options generally ends up being
a bad idea in retrospect.

Inverse boolean options are also not very desirable.

I'd suggest a "auto-connect-ports" boolean option, which defaults to
enabled.


 - auto-connect-ports == true,  connect-ports= <unset>

    => use the proposed default regex for connect ports

 - auto-connect-ports == true, connect-ports = regex

    => use the connect-ports regex

 - auto-connect-ports == false, connect-ports= <unset>

   => don't auto connect at all

 - auto-connect-ports == false, connect-ports=regex

   => logically inconsistent config, report an error

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 :|

> I'd suggest a "auto-connect-ports" boolean option, which defaults to
> enabled.
> 
>  - auto-connect-ports == true,  connect-ports= <unset>
> 
>     => use the proposed default regex for connect ports
> 
>  - auto-connect-ports == true, connect-ports = regex
> 
>     => use the connect-ports regex
> 
>  - auto-connect-ports == false, connect-ports= <unset>
> 
>    => don't auto connect at all
> 
>  - auto-connect-ports == false, connect-ports=regex
> 
>    => logically inconsistent config, report an error

Well, we need two options then, and have one inconsistent variant.
Can't we just get along with connect-ports alone, by using the empty
string for "do not auto connect" ?

take care,
  Gerd

On Thu, Mar 04, 2021 at 02:12:52PM +0100, Gerd Hoffmann wrote:
> > I'd suggest a "auto-connect-ports" boolean option, which defaults to
> > enabled.
> > 
> >  - auto-connect-ports == true,  connect-ports= <unset>
> > 
> >     => use the proposed default regex for connect ports
> > 
> >  - auto-connect-ports == true, connect-ports = regex
> > 
> >     => use the connect-ports regex
> > 
> >  - auto-connect-ports == false, connect-ports= <unset>
> > 
> >    => don't auto connect at all
> > 
> >  - auto-connect-ports == false, connect-ports=regex
> > 
> >    => logically inconsistent config, report an error
> 
> Well, we need two options then, and have one inconsistent variant.
> Can't we just get along with connect-ports alone, by using the empty
> string for "do not auto connect" ?

Historically we've been unable to distinguish between the <unset> and
<set-to-empty-string> scenarios when parsing CLI args. Maybe that
isn't a problem for -audiodev since we're not using QemuOpts for
parsing ?

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 :|

  Hi,

> > Well, we need two options then, and have one inconsistent variant.
> > Can't we just get along with connect-ports alone, by using the empty
> > string for "do not auto connect" ?
> 
> Historically we've been unable to distinguish between the <unset> and
> <set-to-empty-string> scenarios when parsing CLI args. Maybe that
> isn't a problem for -audiodev since we're not using QemuOpts for
> parsing ?

all qapi opts (including -audiodev) can deal with that just fine as
there is a has_$option bool all optional arguments.

take care,
  Gerd