| Message ID | 20210224191927.19271-1-koalinux@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Autoconnect jack ports by default | expand |
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
> 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
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
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;
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(-)