jack-driver updated

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

jack-driver updated

Stefan Westerfeld
   Hi!

After a few improvements recently added, the jack-driver branch should be ready
to use; I've tested and polished the code to the degree that things should just
work.

Currently, the driver assigns itself the highest priority, but doesn't start a
jack server. This means that

 - if jackd is running: the jack driver will be used automatically
 - if jackd is not running: open will fail, and the next best driver
   gets used instead

I'll go through the things that could be improved. Most of the time there is a
corresponding FIXME in the code. Although there are some things that could be
done better, 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.

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

2.) keeping the connection open

Currently, we reconnect to the JACK server every time the user presses play.
Since we auto-connect to the hardware device, this should be ok for simple
cases. However, if the user wishes to connect the BEAST input/output to one or
more programs (visualization or effects), he will have to do it over and over
again each time he presses play.

Most other programs do not behave this way, but keep the connection to the jack
server open all the time; this way connections set up between JACK clients will
be persistent across play|stop. This probably requires deeper changes in BEAST.

3.) jack device latency

The driver API is suboptimal here (see FIXME).

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. BEAST will stay responsive, so the user can
press stop, and next time he presses play the next-best driver would be used.

However, it would be better if BEAST reported something like "JACK server was
shutdown" and automatically stopped "play" mode instead of hanging.

It could be done by changing the check_io function, so that an error code could
be reported.

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.

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.

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.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: jack-driver updated

Tim Janik-6
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.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: jack-driver updated

Stefan Westerfeld
   Hi!

On Tue, Aug 22, 2017 at 11:44:01AM +0200, Tim Janik wrote:
> 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!

From your comments I believe that you didn't review the latest greatest version
of the code, which would be:

https://github.com/swesterfeld/beast/blob/jack-driver/drivers/bsepcmdevice-jack.cc

Why do I think that?

> * assert_return() instead of g_return*_if_fail

This has been fixed.

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

This FIXME doesn't exist in the current version.

> [...]
> - "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

This FIXME doesn't exist either in the current version.

I stopped comparing your comments with my code at this point, assuming that you
didn't test the latest version of the jack driver.

If you used an old version of the jack driver, this could explain the stability
issues you had. Let me know if you can still reproduce crashes with the current
version.

And yes, I know there were valid comments in your email, that still apply to
the current state of the jack driver, so I should try to improve the driver
further based on this. With this mail I'm just trying to be sure that we speak
about the same version of the code.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: jack-driver updated

Tim Janik-6
Hey Stefan,

this is the version you posted to the list and I was testing:
  https://github.com/swesterfeld/beast/tree/jack-driver

On 23.08.2017 15:56, Stefan Westerfeld wrote:
>>From your comments I believe that you didn't review the latest greatest version
> of the code, which would be:
>
> https://github.com/swesterfeld/beast/blob/jack-driver/drivers/bsepcmdevice-jack.cc

Thanks, but I need a proper branch pointer that I can rebase.
I don't care if you rebase the "jack-driver" branch or rename it "wip" or
somesuch, but please give me a branch to check out, so I can rebase and have
easy access to the history.

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: jack-driver updated

Stefan Westerfeld
   Hi!

On Wed, Aug 23, 2017 at 06:46:24PM +0200, Tim Janik wrote:
> this is the version you posted to the list and I was testing:
>   https://github.com/swesterfeld/beast/tree/jack-driver

That is the correct branch. But still there is something wrong with your checkout.

$ git clone [hidden email]:swesterfeld/beast.git
...
$ cd beast
$ git checkout jack-driver
Branch jack-driver set up to track remote branch jack-driver from origin.
Switched to a new branch 'jack-driver'
$ egrep '(g_return_if_fail|assert_return)' drivers/bsepcmdevice-jack.cc
  assert_return (!self->jack_client, false);
  assert_return (self->jack_client != NULL);
  assert_return (self->jack_client, devices);
  assert_return (jack->input_ports.size() == n_channels, 0);
  assert_return (jack->output_ports.size() == n_channels, 0);
      assert_return (frames_written == n_frames, 0); // we checked the available space before
      assert_return (read_frames == n_frames, 0); // we checked the available space before
  assert_return (jack->jack_client != NULL, false);
  assert_return (jack->jack_client != NULL, 0);
  assert_return (jack->jack_client != NULL, 0);
  assert_return (frames_read == handle->block_length, 0);
  assert_return (jack->jack_client != NULL);
      assert_return (jack->device_read_counter == jack->device_write_counter);
  assert_return (frames_written == handle->block_length);

As you see, there is no g_return_if_fail in this version of the code. Yet in
your review you say it needs to be fixed. See my other mail for more examples
of differences between the latest version of my jack-driver branch and the code
review you gave.

> On 23.08.2017 15:56, Stefan Westerfeld wrote:
> >>From your comments I believe that you didn't review the latest greatest version
> > of the code, which would be:
> >
> > https://github.com/swesterfeld/beast/blob/jack-driver/drivers/bsepcmdevice-jack.cc
>
> Thanks, but I need a proper branch pointer that I can rebase.
> I don't care if you rebase the "jack-driver" branch or rename it "wip" or
> somesuch, but please give me a branch to check out, so I can rebase and have
> easy access to the history.

The branch jack-driver is the branch you need to use.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: jack-driver updated

Tim Janik-6
On 24.08.2017 11:42, Stefan Westerfeld wrote:
>    Hi!
>
> On Wed, Aug 23, 2017 at 06:46:24PM +0200, Tim Janik wrote:
>> this is the version you posted to the list and I was testing:
>>   https://github.com/swesterfeld/beast/tree/jack-driver
>
> That is the correct branch. But still there is something wrong with your checkout.

I presume you refer to commit 4d2ad83e87b40b668909028c8b61cd2f0feb3ee6 ?
(Sometimes it helps to use actual commit ids ;-)

> $ git clone [hidden email]:swesterfeld/beast.git
> ...
> $ cd beast
> $ git checkout jack-driver
> Branch jack-driver set up to track remote branch jack-driver from origin.
> Switched to a new branch 'jack-driver'
> $ egrep '(g_return_if_fail|assert_return)' drivers/bsepcmdevice-jack.cc
>   assert_return (!self->jack_client, false);

Thanks, *now* I'm seeing the same thing with just:

        git fetch swesterfeld jack-driver:jack-driver

> The branch jack-driver is the branch you need to use.

Yes, thank you. I got that and can also rebase on master without conflicts.

Still, please go back to my email and fix the issues that are *still* remaining,
e.g. "the libjack dependency must be optional" or the various FIXMEs that I
commented on.

>
>    Cu... Stefan
>

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: jack-driver updated

Stefan Westerfeld
In reply to this post by Tim Janik-6
   Hi!

On Tue, Aug 22, 2017 at 11:44:01AM +0200, Tim Janik wrote:

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

Ok. I did that and fixed make check for the jack driver now.

> 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

Fixed. Using jack_port_get_latency_range() now, which is not deprecated. While
I was at it, also added a latency callback, so BEAST sets the correct the
latency (with ringbuffer) on its input/output ports.

> 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

Nothing to do here (fixed already), this is just because you reviewed an old
version of the driver.

> * Bse::printerr instead of g_print*

Fixed.

> * member_ instead of m_member variables

Right, I fixed this now.

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

Done. You can compile BEAST now with --without-jack to disable the jack driver
explicitely. If you specify nothing, the jack driver will be built if
pkg-config finds libjack*.

I added a status message at the end of configure, so the user can see which
optional dependencies will be built (SpectMorph will be added to that list).

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

Among all linux users, you may be right. But we target a special group of users,
musicians, and in the linux world these are more likely to already use jackd.

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

I don't see this issue here. All my tests (also killing jackd) have shown that
the driver doesn't crash. Maybe it is because you reviewed an old version. Can
you reproduce with the latest jack-driver branch version?

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

SIGPIPE is triggered if we write() data to a process we communicate with, and
that process dies. This is what can happen if we are connected to jackd, jackd
dies, and we use jack_*() functions after that.

I've done more tests, and was not able to reproduce the problem when I remove
the SIGPIPE code from our jack driver. Not blocking SIGPIPE doesn't lead to
crashes (like it did when I originally wrote the jack driver).

Nowadays, Gtk unconditionally ignores SIGPIPE signals. See gtk/gtkmain.c, which
contains

  signal (SIGPIPE, SIG_IGN);

This alone is enough to make beast perfectly stable, even if jackd dies, as we
initialize Gtk before that. So if you do

$ kill -13 <beast_pid>    # 13 = SIGPIPE

with a vanilla BEAST, nothing happens. So the JACK driver should be stable whether
or not we add more code.

I wasn't so happy that we depend on another library doing this for us (for
instance, what happens if we write a commandline program that interacts with
the JACK driver?). Also note that the manual page for signal() says that

... The effects of signal() in a multithreaded process are unspecified.

So I removed the SIGPIPE code from the JACK driver and added code to libbse in
bsemain.cc which blocks SIGPIPE explicitely. It should be early enough so that
it happens before other threads are initialized, so that all of our threads
inherit the signal mask.

As a final remark, from looking at the source code, it seems that the jackd2
appears to have some SIGPIPE handling code, whereas jackd1 doesn't. So this
may be a non-issue as long as the user runs jackd2.

> - jack_client_open ("beast" <- no translation needed

Right. Not mentioned any more.

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

Device unloading is already gone, I haven't seen that BEAST would unload the
driver (you reviewed old code which still has this FIXME). However it seems
that the right way to do it is the one from our last phonecall, that is: BEAST
itself should keep the driver open between explicit

 - start audio engine
 - stop audio engine

so this is not really a JACK driver issue, but should be fixed indepently of
the driver the user uses. So I removed the FIXME from the driver code.

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

I don't think we can do much better latency wise. We have these ring buffers,
and these need to contain a minimum amount of data to be able to always satisfy
the JACK callback.

The only alternative way to implement things would be without ring buffers. We
would directly run the engine within the JACK callback itself. So whenever
JACK thinks we should produce (for instance) 256 samples, we would compute
these samples. This would allow us to do zero latency. However, we would
explicitely violate the rules JACK has for code that runs within the callback,
which the JACK developer docs state as

...The code in the supplied function must be suitable for real-time execution.
That means that it cannot call functions that might block for a long time. This
includes all I/O functions (disk, TTY, network), malloc, free, printf,
pthread_mutex_lock, sleep, wait, poll, select, pthread_join, pthread_cond_wait,
etc, etc.

And of course since our driver API is just not designed this way, we would need
deeper changes in BEAST to get callback driven audio to work.



Anyway, lets go back to what we currently have: the user will specify the
amount of buffering within BEAST. This cannot be arbitarily small, because
the ringbuffers within BEAST must be big enough to ensure that the JACK
callback will have enough samples available when it occurs.

The jack latency will be added to the size of the ringbuffer yielding the
total value for the buffer. Now what is the minimum obtainable latency? This
depends on the jackd period size. I did an experiment with a deliberately small
period size:

$ jackd -d alsa -p 128

and a small latency setting in BEAST of 5ms. Then my jack driver reports

rlatency=2.667 ms wlatency=5.333 ms ringbuffer=7.000 ms total_latency=15.000 ms

So the effective output latency is the size of the ringbuffer + the jack write
latency, that is 12.33 ms. Note however that although I could play the demo
song with these settings, I observed occasional xruns. So this is about as
little latency as possible with my system.

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

Yes, this FIXME is about input. BEAST will in some situations *read* data from
the JACK driver, in other situations it won't. This even can change while the
driver is open, like for instance if you remove the input module from the full
duplex example.

However, this FIXME is fixed already: the code in jack_device_write() will now
trigger reading if necessary. Here is the comment from the code

  /* our buffer management is based on the assumption that jack_device_read()
   * will always be performed before jack_device_write() - BEAST doesn't
   * always guarantee this (for instance when removing the pcm input module
   * from the snet while audio is playing), so we read and discard input
   * if BEAST didn't call jack_device_read() already
   */

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

No. Basically we could in some cases use faster (de)interleaving code. We could
even when redesigning the driver API avoid interleaving and deinterleaving because
the input/output module in the engine already has seperate buffers for each channel
which is just the memory layout the JACK callback needs/produces.

However, IMHO the total cost of the interleaving/deinterleaving compared to the
total cost of synthesis itself make this an optimization that we shouldn't do.

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

Ok, I added a Bse::printerr for the case that jackd dies. Ideally this would
stop the playback, too and popup a dialog, but I don't know how to do that
properly.

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

By the way, this doesn't look like a SIGPIPE issue. SIGPIPE usually causes a
silent and clean exit from the application (no messages). If you still see this
with the latest version, a backtrace would be nice.

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

Yes, this works. At least I can do it without any problems with the current
version of the driver.

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

Done. All errors produced by jack_client_open() are now suppressed. And of
course if errors do occur during JACK driver operation, they will still be
printed.

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

See remarks about SIGPIPE above.

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

Yes, we do. We reconnect on every play.

> > 3.) jack device latency
> > > The driver API is suboptimal here (see FIXME).
>
> I'm happy to review patches here.

I've now implemented that and removed the FIXME. Basically I changed

static guint            jack_device_latency             (BsePcmHandle           *handle);

to

static void             jack_device_latency             (BsePcmHandle           *handle,
                                                         guint                  *rlatency,
                                                         guint                  *wlatency);

in the alsa, jack and null drivers, and the BsePcmHandle code. Turns out that
the API is not used by anyone at the moment, but in any case, this is what the
API should look like.

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

As stated above, we now at least Bse::printerr() a message if the JACK server
dies.

> No, Beast dies and crashes in flames.

Since you tested an old version of the code, I suggest that you try the latest version
of the jack-driver branch here. I can definitely kill the JACK server here, and BEAST
keeps on running.

Also useful information would be the version of JACK you're using, the JACK API you
and version built BEAST against (libjack-dev vs. libjack-jackd2-dev), any stdout/stderr
stuff, and possibly a backtrace.

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

OK, I added the jack_autoconnect setting and use this in the JACK driver now.

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

Ok, after a little research, I now removed the FIXME without changing the code.

Running multiple JACK servers (with different names) is considered an expert only
feature. Neither Ardour nor Qtractor have any code to support this. Users that
really know what they do can set the environment variable JACK_DEFAULT_SERVER
before starting an application like Ardour, to use a non-default jackd.

This environment variable works with BEAST, too (I tested it), and from all I
know now this should be good enough.

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

Ok, using Bse::printerr now.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast