Re: [PATCH v7] audio/jack: add JACK client audiodev

[Geoffrey McRae]

On 2020-05-08 16:34, Markus Armbruster wrote:
> Geoffrey McRae <address@hidden> writes:
>
> > On 2020-05-06 16:06, Markus Armbruster wrote:
> > > You neglected to cc: the audio maintainer.  Doing that for you now.
> > > You
> > > can use scripts/get_maintainer.pl to find maintainers.
> >
> > Thanks, I was unaware.
>
> Happy to help :)
>
> > > Find my QAPI schema review inline.
> >
> > Ditto
> >
> > > Geoffrey McRae <address@hidden> writes:
> > >
> > > > This commit adds a new audiodev backend to allow QEMU to use JACK as
> > > > both an audio sink and source.
> > > >
> > > > Signed-off-by: Geoffrey McRae <address@hidden>
> > > > ---
> > > >  audio/Makefile.objs    |   5 +
> > > >  audio/audio.c          |   1 +
> > > >  audio/audio_template.h |   2 +
> > > >  audio/jackaudio.c      | 677
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > >  configure              |  17 ++
> > > >  qapi/audio.json        |  56 +++-
> > > >  6 files changed, 756 insertions(+), 2 deletions(-)
> > > >  create mode 100644 audio/jackaudio.c
> [...]
> > > > diff --git a/qapi/audio.json b/qapi/audio.json
> > > > index c31251f45b..bdb0552d15 100644
> > > > --- a/qapi/audio.json
> > > > +++ b/qapi/audio.json
> > > > @@ -152,6 +152,55 @@
> > > >      '*out':     'AudiodevPerDirectionOptions',
> > > >      '*latency': 'uint32' } }
> > > >
> > > > +##
> > > > +# @AudiodevJackPerDirectionOptions:
> > > > +#
> > > > +# Options of the JACK backend that are used for both playback and
> > > > +# recording.
> > > > +#
> > > > +# @server-name: select from among several possible concurrent
> > > > server instances.
> > > > +# If unspecified, use "default" unless $JACK_DEFAULT_SERVER is
> > > > defined in the
> > > > +# process environment.
> > >
> > > Suggest something like
> > >
> > >    # (default environment variable $JACK_DEFAULT_SERVER if set, else
> > >    # "default").
>
> Uh, needs a colon after "(default" for clarity.
>
> > >
> >
> > I'd be happy to change this, however, this terminology reflects the
> > JACK documentation which people already using JACK will be familiar
> > with.
>
> There's a tension between your (sensible) desire to stay close to JACK
> documentation, and our need for reasonably consistent QMP reference
> documentation.
>
> Where we're talking about genuine JACK stuff, staying close to JACK
> documentation feels more important.
>
> Where we're doing general QMP things, doing them the established QMP 
> way
> feels more important.
>
> Phrasing "this is the optional member's default value" is QMP.  We do 
> it
> all over the place like (default: WHATEVER).  Please stick to this
> conventional QMP format.
>
> The WHATEVER is JACK stuff.  Feel free to improve on my suggested
> phrasing there.

Not a problem, I will reword this as requested.

> > > > +#
> > > > +# @client-name: the client name to use. The server will modify
> > > > this name to
> > > > +# create a unique variant, if needed unless @exact_name is true.
> > >
> > > Do we really need this much magic?
> > >
> > > What would we lose with just @client-name?  If it's present, use it 
> > > as
> > > is (no magic), else make up a client name.
> >
> > There is no magic on our part, this is how the JACK server works and
> > is the default.
>
> Aha!
>
> Please bear with my ignorant questions...
>
> QEMU doesn'r have to expose everything the JACK library and server
> provide, only the stuff that is useful to QEMU's users.
>
> There seem to be several variations:
>
> * @client-name absent, @exact_name false
>
>   JACK picks a name.
>
> * @client-name absent, @exact_name true
>
>   Error?  @exact_name silently ignored?

The @client-name is set to the VM's name as the default if absent.

> * @client-name present, @exact_name false
>
>   JACK picks a name, using @client-name as inspiration somehow.

JACK picks a name only if there is a name clash, a duplicate, otherwise, 
it honours the requested name.

> * @client-name present, @exact_name true
>
>   JACK uses @client-name if possible, else error.
>
> Use cases for the three variations that make sense?

JACK is extremely configurable and is used in both general-purpose audio 
through to professional audio mastering and distribution in conferences 
and stadiums. As such all these use cases are completely valid and need 
to be present otherwise it limits the usefulness of this audio dev. The 
plumbing of jack is entirely left up to the user and as such there are 
countless different possible configurations and use cases.

> When letting JACK pick a name, how do users learn the name?

We print it out if jack did this:

    if (status & JackNameNotUnique) {
        dolog("JACK unique name assigned %s\n",
          jack_get_client_name(c->client));
    }

The user can also learn the name through various methods such as running 
`jack_lsp` or using `qjackctl`, or any number of potential jack 
plugboards such as `Carla`, etc.

> > > > +#
> > > > +# @connect-ports: if set, a regular expression of port name(s) to
> > > > match to auto
> > > > +# connect to at startup.
> > >
> > > Pardon my ignorance... where do the port names being matched come 
> > > from?
> >
> > Other JACK client ports. QEMU become a JACK client which has audio
> > ports and can be patched into the system sound device, or any number
> > of other ports for DSP.
>
> I think a slightly less terse doc comment would be a lot easier to
> understand.  At least "s/ of port name(s)/of JACK client port name(s)/.
> Also, "to auto connect to" is quite vague on what gets connected to
> these ports.  Can we do a bit better?

The wording may need correcting however as jack is so flexible, what it 
gets connected to can not be stated. It could be anything depending on 
what clients/plugins the user are using already, the jack allows for a 
very complex and complete audio system. I have attached a screenshot of 
my JACK graph for a simple example of what this looks like.

> > > > +#
> > > > +# @start-server: set to true to start a jack server instance if
> > > > one is not
> > > > +# present.
>
> Default?

False :), I will add this.

> > > Is this an external server program?
> >
> > Yes, the jack library can start the jack server much like PulseAudio
> > auto spawns a process on most systems. People using JACK will be
> > familiar with this already.
>
> Replace "server instance" by "server process"?

I have no issue with this, however, FYI there can be multiple jack 
instances running with different server names managing different sound 
devices. Please let me know which you would like as I have no opinion in 
either instance.

> > > > +#
> > > > +# @exact-name: use the exact name requested otherwise JACK
> > > > automatically
> > > > +# generates a unique one, if needed.
>
> Default?

False :), I will add this

> > > > +#
> > > > +# Since: 5.1
> > > > +##
> > > > +{ 'struct': 'AudiodevJackPerDirectionOptions',
> > > > +  'base': 'AudiodevPerDirectionOptions',
> > > > +  'data': {
> > > > +    '*server-name':   'str',
> > > > +    '*client-name':   'str',
> > > > +    '*connect-ports': 'str',
> > > > +    '*start-server':  'bool',
> > > > +    '*exact-name':    'bool' } }
> > > > +
> > > > +##
> > > > +# @AudiodevJackOptions:
> > > > +#
> > > > +# Options of the JACK audio backend.
> > > > +#
> > > > +# @in: options of the capture stream
> > > > +#
> > > > +# @out: options of the playback stream
> > > > +#
> > > > +# Since: 5.1
> > > > +##
> > > > +{ 'struct': 'AudiodevJackOptions',
> > > > +  'data': {
> > > > +    '*in':  'AudiodevJackPerDirectionOptions',
> > > > +    '*out': 'AudiodevJackPerDirectionOptions' } }
> > > > +
> > > >  ##
> > > >  # @AudiodevOssPerDirectionOptions:
> > > >  #
> > > > @@ -297,11 +346,13 @@
> > > >  #
> > > >  # An enumeration of possible audio backend drivers.
> > > >  #
> > > > +# @jack: JACK audio backend (since 5.1)
> > > > +#
> > > >  # Since: 4.0
> > > >  ##
> > > >  { 'enum': 'AudiodevDriver',
> > > > -  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'oss', 'pa',
> > > > 'sdl',
> > > > -            'spice', 'wav' ] }
> > > > +  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss',
> > > > 'pa',
> > > > +            'sdl', 'spice', 'wav' ] }
> > > >
> > > >  ##
> > > >  # @Audiodev:
> > > > @@ -327,6 +378,7 @@
> > > >      'alsa':      'AudiodevAlsaOptions',
> > > >      'coreaudio': 'AudiodevCoreaudioOptions',
> > > >      'dsound':    'AudiodevDsoundOptions',
> > > > +    'jack':      'AudiodevJackOptions',
> > > >      'oss':       'AudiodevOssOptions',
> > > >      'pa':        'AudiodevPaOptions',
> > > >      'sdl':       'AudiodevGenericOptions',

2020-05-08-171011_1666x552_scrot.png
Description: PNG image