Re: jack-driver updated

[Tim Janik <timj gnu org>]

Hey Stefan.

On 16.07.2017 15:42, Stefan Westerfeld wrote:
>  IMHO none of these issues is so critical that it is a blocker for
> merging. I.e. even if we ship jack-driver as-is, we're still better off than
> without it.

Wrong. Sorry, there's no easier way to put it. The current shape of the jack
driver comes close to an end user assault and the world needs to be saved from it!

With that off my chest, let me try to be constructive ;-)

The Good:
We have a jack driver now, that means in theory the pitiful fraction of mistaken
linux users that think jackd would actually benefit them, instead of eating
their latency and stabbing their stability are now able to run Beast along side
their bug ridden jack related software stack.

The Bad:
The code quality of the BSE jack driver leaves lots of things to wish for, at
the very least, please run 'make check' on the Beast tree when you hack on code.
The check routines have been highly tuned not to waste too much developer
patience. I know your laptop is faster than mine, and it takes merely 25 seconds
here to run check + installcheck, you can afford that investment before
requesting a merge.
While we're at it, the deprecations during compilation also need fixing:
        jack_port_get_total_latency(jack_client_t*, jack_port_t*)' is deprecated
There're like a dozen FIXMEs in the code, kudos for adding the comments that
point at areas that need improvements, but hey, we really have to have those
fixed before we can merge and ship.
A lot of places need smaller updates to comply with the coding standards
elsewhere in BSE, I saw you made a start of that already, but what's still
missing is, of the top of my head:
        * assert_return() instead of g_return*_if_fail
        * Bse::printerr instead of g_print*
        * member_ instead of m_member variables
Also the libjack dependency must be optional. The peaceful living individuals
that have no clue about jackd or libjack-jackd2-dev should not be confronted
with a hard libjack dependency that breaks Beast builds without providing any
benefit for them.
For the record, the *VAST* majority of linux users these days can be expected to
have pulseaudio installed, they wouldn't even be able to run the latest Firefox
if they didn't. The same can not be said of jackd, so in a purely utilitarian
world, it'd make a lot more sense to spend effort on improving pulseaudio
support than jackd support.

The Ugly:
Beast crashes when Jackd exits (or crashes).
Even if Beast is *not* playing audio, the moment jackd exits, all user data held
by any currently running Beast instance is destroyed in flames. This is why we
definitely are better off *without* it, stability is paramount and this needs to
be fixed before end users can try the jack driver.

The Details:

- Why is SIGPIPE blocked? *If* blocking SIGFPE for our engine threads from jack
driver initialization works at all, then by pure luck. If needed, this'd have to
be moved to engine thread creation.

- jack_client_open ("beast" <- no translation needed

- "FIXME: get rid of device unloading, so that the jackd connection can be kept
initialized" <- send patches

- "FIXME: should we try to tweak local buffering so that it corresponds to the
user defined latency" <- Yes, I found myself adjusting the Beast latency
settings with jackd and it had no effect. Also, if latency is / stays huge with
jackd, that totally defeats running it in the first place.

- "FIXME no guarantee that beast will read the data from jack since this depends
on the presence of a module that reads data the synthesis graph" <- There is no
way a synthesis graph is executed without output modules, output modules are the
only thing driving the engine

- "/* get rid of this *slow* interleaving code */" <- seems this should be a FIXME ?

- "FIXME: we need a way to indicate an error here" <- at the very least, *print*
a message, so developers can trace the error conditions mentioned in the code

- If jackd stops (or crashes), Beast crashes (even if not playing), loosing all
user data:
  beast-0.11.1: ../nptl/pthread_mutex_lock.c:81: __pthread_mutex_lock: Assertion
`mutex->__data.__owner == 0' failed.

- Beast should reconnect to jackd when it starts playing, and completely
disconnect from jack on stop, so the following sequence works:
        Beast plays through pulseaudio
        Beast stops
        pasuspend -- jackd
        Beast plays through jackd
        Beast stops
        jackd exits
        Beast plays through pulseaudio again

- If jackd is not running and Beast plays through PA, the jack driver still
spams stderr with error messages, please silence the driver if its not working.

In summary, the driver needs a lot more love from you and fixes before it can be
considered for inclusion. The FIXMEs need to be straightened out one way or the
other - it's ok if other parts of BSE need touching for that (e.g. to mask
signals in the engine threads or to have a BSE callback to stop playback
altogether).


> 1.) signal handling
>
> SIGPIPE must be blocked in our thread, because otherwise if the jack server
> dies, BEAST will die receiving SIGPIPE - see FIXME - this should not be done
> in our driver code, but BEAST should setup SIGPIPE handling globally

I can't reproduce this atm, Beast simply crashes when Jackd dies.

> 2.) keeping the connection open
>
> Currently, we reconnect to the JACK server every time the user presses play.

No we don't. That's what we *should* do.

> 3.) jack device latency
> > The driver API is suboptimal here (see FIXME).

I'm happy to review patches here.

> 4.) handling jack server down
>
> It can happen that during play, the JACK server dies (user can manually stop
> server, server can crash). When that happens, currently the play position
> pointer will no longer advance.

No, Beast dies and crashes in flames.

> 5.) auto connect should be configurable
>
> Auto connecting to the hardware device (which is what the driver currently
> does) is often but not always the desired thing to do. Other JACK applications
> allow disabling auto connect, so that external programs (such as qjackctl) can
> manage connections for the client.
>
> So we should have a way of somehow disabling auto connect. Ideally at the UI.

Can you add a BSE setting for this, or is anything else needed?

> 6.) jack server name
>
> We currently don't support connecting to a specific JACK server, if more than
> one is running. See FIXME. Ideally this could be added at the UI.

What's the use case for multiple jackd here? What do other jackd clients do?

> 7.) xrun reporting
>
> We currently report xruns (for our internal ringbuffer) on stdout. Not sure if
> that is the right thing to do, maybe we should just ignore xruns alltogether or
> report them using PDEBUG() - so that developers can see them, but not users.

Nope, use printerr for now. Users *hear* xruns, and prining a message gives them
a clue about the cause.

>    Cu... Stefan

PS: Take this with a grain of salt, every once in a while I try to open up my
mind and give jack another chance. Each time I do that, I run into major issues
starting it, become annoyed by the bunch of error messages I'm confronted with,
am disgusted by the qjackctl UI and get frustrated because jackd causes latency,
crashes and takes all user data with it.

-- 
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.