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

[Markus Armbruster]

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.

>>> +#
>>> +# @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?

* @client-name present, @exact_name false

  JACK picks a name, using @client-name as inspiration somehow.

* @client-name present, @exact_name true

  JACK uses @client-name if possible, else error.

Use cases for the three variations that make sense?

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

>>> +#
>>> +# @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?

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

Default?

>>
>> 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"?

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

Default?

>>> +#
>>> +# 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',