Re: bsepcmdevice-jack.cc (Re: Beast Jack driver)

[Stefan Westerfeld <stefan space twc de>]

   Hi!

Same comment as for the other reply: I'll post fixed code later, so I'll
just answer to discussion stuff.

On Fri, Jun 02, 2006 at 01:45:01AM +0200, Tim Janik wrote:
> >enum
> >{
> > CALLBACK_STATE_INACTIVE = 0,
> > CALLBACK_STATE_ACTIVE = 1,
> > CALLBACK_STATE_PLEASE_TERMINATE = 2,
> > CALLBACK_STATE_TERMINATED = 3
> 
> minor cosmetic, we usually align '='-symbols in enums.
> 
> >};
> 
> you should provide a type name for this enum, which 
> can be used above to tell apart valid states of your
> callback_state variable.

The problem is that callback_state is an atomic integer, so I think it
needs to be volatile int, and I can't declare it of the enum type; or am
I wrong?

> > vector<jack_port_t *>	  input_ports;
> > vector<jack_port_t *>	  output_ports;
> >
> > guint			  buffer_frames;	/* input/output 
> > ringbuffer size in frames */
> >
> > Cond			  cond;
> > Mutex			  cond_mutex;
> 
> erk, what exactly is protected by this cond and this mutex?
> something like this definitely needs to be outlined in code
> (in comments or by naming prefixes, look for BirnetMutex
> in bseengineutils.c to see examples for that)

Well, I'll put a comment there, but if you want to know what it does
right now: the condition is signalled whenever the realtime callback has
been called and is thus be used to wait for events that depend on the
realtime callback (like waiting for termination or new data).

The mutex is mostly useless - its there because conditions require it
(you can't wait on a condition without one), but all code needs to work
without the mutex, because the realtime thread can not use lock() (only
trylock()) due to possible priority inversion.

> > jack_status_t status;
> >
> > self->jack_client = jack_client_open ("beast", JackNoStartServer, 
> > &status); /* FIXME: translations(?) */
> 
> what's this "beast" being used for? (especially since you
> quesiton translation)
> e.g. if it's similar to window titles, it should be "BEAST",
> if it's going to be used as a program name in continueous text,
> it should prolly be _("Beast") and if its meant as a technical
> identifier (e.g. for serialization), it should be "beast".
> 
> maybe Paul can shed light on this?

Its the name of the client, which means that for the command line utils
for instance you'll use

beast:in_1

as port name. So this suggests that you may not want to translate it.
However, for qjackctl, its also the label that is displayed in the GUI,
in the widget where the clients (and ports) are listed.

So this suggests that you may want to translate it. However then, if you
store a session (somehow, I don't know the details), and try to restore
it in a different language, it will break.

So probably its just that jack has no model that strings might need to
be translated, and just treats the world as if there was only one
language, so the best we can do is to not translate the name.

> > DEBUG ("attaching to JACK server returned status: %d\n", status);
> > /* FIXME: check status for proper error reporting
> 
> doesn't teh jack API have something like:
>   const char* jack_status_to_string (jack_status_t status);
> ?

No.

> >static map<string, DeviceDetails>
> >query_jack_devices (BsePcmDeviceJACK *self)
> >{
> > map<string, DeviceDetails> devices;
> >
> > /* FIXME: we need to make a difference here between
> >  *  - jackd is running, but no output ports can be found
> 
> curious, why exactly could that be the case?

I am not sure it can happen, as you need to load some driver to be able
to start jack at all, and the driver will probably always register some
terminal ports. However, in theory it could be possible to start jack
only to connect two applications; for instance one that produces audio
(beast) and another one that sends it over the net somewhere. Then,
which application you start first is arbitary, and it might as well be
that beast gets started first.

But... well... I don't know if jack is ever going to support starting
without a driver (it would need to do something to get the timing,
sample rate and so on setup; normally probably the driver does this).

> >  *  - jackd is not running
> 
> that should/will actually be determined by jack_client_open(), right?

Right. Then self->jack_client will be NULL. Or not, if jackd was once
running but crashed later...

> >	      DeviceDetails &details = devices[device_name];
> >	      details.ports++;
> 
> why are you incrementing here already, if the port
> could possibly not be looked up by name?

Nothing of significance. If jack_port_by_name returns NULL, the port
doesn't exist any longer (or jackd crashed), and it will not be used
anyway, since the other flags don't say that it is usable either as
input or as output.

> >	      jack_port_t *jack_port = jack_port_by_name (self->jack_client, 
> >	      jack_ports[i]);
> >	      if (jack_port)
> >		{
> >		  int flags = jack_port_flags (jack_port);
> >		  if (flags & JackPortIsInput)
> >		    {
> >		      details.input_ports++;
> >		      details.input_port_names.push_back (jack_ports[i]);
> >		    }
> >		  if (flags & JackPortIsOutput)
> >		    {
> >		      details.output_ports++;
> >		      details.output_port_names.push_back (jack_ports[i]);
> >		    }
> >		  if (flags & JackPortIsPhysical)
> >		    details.physical_ports++;
> >		  if (flags & JackPortIsTerminal)
> >		    details.terminal_ports++;
> 
> why are you counting physical_ports and terminal_ports here,
> regardless of whether the port in question is an input,
> output or neither port?
> and, if this is purely statistical/informative counting,
> why are you only counting ports that can be looked up by name?
> 
> /me scratches head...

I can only gather statistics if I get the jack_port (because only then
jack_port_flags can be called). I gather statistics to display it later.
Intersting is the physical flag "hardware ports, like an alsa device",
and the input/output flag. I am just gathering terminal for the sake of
completeness.

> >		}
> >	    }
> >	}
> >     free (jack_ports);
> >   }
> >
> > return devices;
> 
> ugh, isn't this a full fledged recursive map+strings+DeviceDetails copy?

Yes. So? Its not in any time critical code, so I don't bother to
optimize it.

> >}
> >
> >static SfiRing*
> >bse_pcm_device_jack_list_devices (BseDevice *device)
> >{
> > BsePcmDeviceJACK *self = BSE_PCM_DEVICE_JACK (device);
> > map<string, DeviceDetails> devices = query_jack_devices (self);
> >
> > SfiRing *ring = NULL;
> > for (map<string, DeviceDetails>::iterator di = devices.begin(); di != 
> > devices.end(); di++)
> >   {
> >     const string &device_name = di->first;
> >     DeviceDetails &details = di->second;
> >     BseDeviceEntry *entry;
> >     entry = bse_device_group_entry_new (device, g_strdup 
> >     (device_name.c_str()),
> >                                                 g_strdup ("JACK Devices"),
> >                                                 g_strdup_printf ("%s 
> >                                                 (%d*input %d*output)%s",
> >								   device_name.c_str(),
> >								   details.input_ports,
> >								   details.output_ports,
> >								   details.physical_ports == details.ports ?
> >								     " [ 
> >								     hardware ]" : ""));
> 
> hm, can you please elaborate the
>   details.physical_ports == details.ports ?  " [ hardware ]" : ""
> condition for me? ;)

Well... if all ports of a client are physical, then its got to be a
hardware device. For instance alsa_pcm has only physical ports here.

I am not sure what a client is that has *some* physical ports... I've
yet to see one.

> >
> >     /* ensure that alsa_pcm always is the first item listed (it is the 
> >     default device) */
> 
> hm, doesn't jack provide some kind of rating for the devices to pick?

No. People hardcode alsa_pcm as default all day long.

> >      *
> >      * normal operation:
> >      *
> >      * [bse thread]      writes some data to output_ringbuffer
> >      * [bse thread]      checks remaining space, there is no room left
> >      * [bse thread]      sleeps on the condition
> 
> BSE never sleeps, waiting for audio drivers. take a look at
> alsa_device_write() and alsa_device_check_io() in bsepcmdevice-alsa.c.
> i.e. allow full buffer writes if at all possible and provide
> timeout information on how long it takes for enough buffer space
> to be available.
> that's the kind of semantic that needs to be implemented.

Yes, it does block in the ALSA driver. However, I think I'll come back
to it in a seperate mail, since you said it shouldn't or maybe should,
so I'll investigate.

> >      * [bse thread]      writes some data to output_ringbuffer
> >      * [bse thread]      checks remaining space, there is no room left
> >      * [jack thread]     reads some data from output_ringbuffer
> >      * [jack thread]     signals the condition
> >      * [bse thread]      sleeps on the condition
> 
> such races are only possible when not using conditions properly.
> you always have to follow the sequence: lock(); check(); wait(); unlock();
> and on the pther side: lock(); update(); signal(); unlock();
> this ensures that no update/signal goes by unnoticed by the check. see:
>   http://developer.gnome.org/doc/API/2.0/glib/glib-Threads.html#GCond

Cool. Just that I can't lock() in the RT thread, because of priority
inversion.

I've read a little kernel code today, and fs/pipe.c has a
PIPE_MUTEX(...) which is being locked. So I am not sure I can use
write() to wakeup the master thread either.

However pthread_cond_signal in glibc also seems to have a kind of a
mutex (so its also a problem), although I am not sure if I interpret the
code correctly. But if I do, then I am running out of ideas how to get
this right at all.

Well, I think I have to think/read/ask others more about it until I know
whats the right thing to do, to get the synchronization between the
engine thread and the jack realtime callback right.

> >      * since the jack callback gets called periodically, the bse thread 
> >      will
> >      * never starve, though, in the worst case it will just miss a frame
> >      *
> >      * so we absolutely exclude the possibility for priority inversion (by
> >      * using trylock instead of lock), by introducing extra latency in
> >      * some extremely rare cases
> 
> apart from excessive newlines ;) this argumentation is faulty.
> think of jack exiting after you hit your signal-lost period.
> BSE will hang forever, unacceptable.

Won't because all conditions are timeout bound and there is a shutdown
handler that libjack will call if jack dies, so that eventually
jack->is_down gets set, upon which nothing will hang (look at the code
again to see for yourself).

> 
> >      */
> >     if (have_cond_lock)
> >	jack->cond_mutex.unlock();
> >   }
> > else
> >   {
> 
> when exactly is this branch tirggered? (lost track due to the excessive
> inlined commenting above.)

There are different activation states. Thats the case where the callback
has marked to be inactive (shouldn't touch ringbuffers and such), so it
will just write out zero blocks.

> isn't this the !trylock() case?
> the purpose of having an atomic ringbuffer in the first place was to
> not be forced into using lock/trylock/unlock in the jack thread.
> so this if-branch shouldn't occour in the implementaiton at all.

Trylock is there to make some stuff (like updating the ringbuffers)
happen within a lock, so that in principle, things that wait on new
ringbuffer state will not fall in the condition trap you mentioned
above. However, since its only a *try*lock, it can't prevent the
condition trap entierly. Thus the lengthly comment.

> as an aside, if we would be willing to pay lock/unlock in the jack thread,
> we could as well pass around list nodes that maintain audio buffers.
> you could get entirely rid fo the value copying in the ring buffer that 
> way...

No, because the engine block size and the jack block size are not
necessarily dividable, so you've got to do some copying to make get
map the "half engine blocks" onto jack blocks, and the "half jack
blocks" onto engine blocks. Also the jack buffers get invalid after the
process() callback, and the engine buffers get invalid out of the engine
process() so you've got to copy the data somewhere.

The only way to get rid of copying entierly would be to run the engine
process cycle in the jack thread, either directly via lock, or with two
blocking context switches back and forth. But that'd probably violate a
million restrictions that apply within the jack callback.


> >     for (guint ch = 0; ch < jack->input_ports.size(); ch++)
> >	{
> >	  sample_t *values = (sample_t *) jack_port_get_buffer 
> >	  (jack->input_ports[ch], n_frames);
> >	  Block::fill (n_frames, values, 0.0);
> >	}
> >     for (guint ch = 0; ch < jack->output_ports.size(); ch++)
> >	{
> >	  sample_t *values = (sample_t *) jack_port_get_buffer 
> >	  (jack->output_ports[ch], n_frames);
> >	  Block::fill (n_frames, values, 0.0);
> >	}
> 
> 
> >   }
> > jack->cond.signal();
> 
> ah, here's the racy signalling *outside* of the mutex lock being held...
> 
> >
> > if (callback_state == CALLBACK_STATE_PLEASE_TERMINATE)
> >   {
> >     Atomic::int_set (&jack->callback_state, CALLBACK_STATE_TERMINATED);
> >     return 1;
> >   }
> > return 0;
> 
> this misses a comment on the purpose of the return value.

FYI:

0 = run on
1 = die now

But I'll add a comment.

> >static void
> >terminate_and_free_jack (JackPcmHandle *jack)
> >{
> > g_return_if_fail (jack->jack_client != NULL);
> >
> > Atomic::int_set (&jack->callback_state, CALLBACK_STATE_PLEASE_TERMINATE);
> > while (Atomic::int_get (&jack->callback_state) == 
> > CALLBACK_STATE_PLEASE_TERMINATE)
> >   {
> >     AutoLocker cond_locker (jack->cond_mutex);
> >
> >     if (jack->is_down) /* if jack is already gone, then forget it */
> >	Atomic::int_set (&jack->callback_state, CALLBACK_STATE_TERMINATED);
> >     else
> >	jack->cond.wait_timed (jack->cond_mutex, 100000);
> >   }
> 
> is the following for real?
> you disconnect the ports *after* the connection is terminated?
> or, is "terminated" wrongly worded here?

Its not the connection that terminated. Its just that the process
callback will no longer called (i.e. the client has become inactive).

In principle, you could start new process callback cycles, by calling
jack_activate again. But can't happen with the driver model bse gives
me.

It can happen, that jackd has crashed inbetween, so I am disconnecting
ports that are no longer there at all. But it seems that libjack can
handle it (by seeing that writing the disconnect requests to the jack
pipe will fail), so in this case it will return an error which I'll
ignore.

> > for (guint ch = 0; ch < jack->input_ports.size(); ch++)
> >   jack_port_disconnect (jack->jack_client, jack->input_ports[ch]);
> >
> > for (guint ch = 0; ch < jack->output_ports.size(); ch++)
> >   jack_port_disconnect (jack->jack_client, jack->output_ports[ch]);
> >
> > jack_deactivate (jack->jack_client);
> >
> > delete jack;
> >}
> >

> > for (guint i = 0; i < handle->n_channels; i++)
> >   {
> >     const int port_name_size = jack_port_name_size();
> >     char port_name[port_name_size];
> >     jack_port_t *port;
> >
> >     snprintf (port_name, port_name_size, "in_%u", i);
> 
> please use g_snprintf() instead.
> 
> >     port = jack_port_register (self->jack_client, port_name, 
> >     JACK_DEFAULT_AUDIO_TYPE, JackPortIsInput, 0);
> 
> what is JACK_DEFAULT_AUDIO_TYPE? shouldn't we request floats here (i
> dunno the jack API though).

JACK_DEFAULT_AUDIO_TYPE is float. There is no other audio type at all,
i.e. there is no negotaition or anything. It also corresponds to my
sample_t typedef above (is now called JackSample typedef to be
UpperLowerCaseNamed), which is also float. Otherwise, the whole thing
will break into pieces anyway, because I'll just copy things.

> > /* activate */
> >
> > jack_set_process_callback (self->jack_client, jack_process_callback, 
> > jack);
> > jack_on_shutdown (self->jack_client, jack_shutdown_callback, jack);
> > jack_activate (self->jack_client);
> 
> you're activating here without checking for if (error)?

Good point. Will fix.

> > /* connect ports */
> >
> > map<string, DeviceDetails> devices = query_jack_devices (self);
> 
> querying the devices could have been done before registering
> the ports above.

So what? I can only connect the ports anyway.

> > map<string, DeviceDetails>::const_iterator di;
> >
> > di = devices.find (dname);
> > if (di != devices.end())
> >   {
> >     const DeviceDetails &details = di->second;
> >
> >     for (guint ch = 0; ch < handle->n_channels; ch++)
> >	{
> >	  if (details.output_ports > ch)
> >	    jack_connect (self->jack_client, 
> >	    details.output_port_names[ch].c_str(), jack_port_name 
> >	    (jack->input_ports[ch]));
> >	  if (details.input_ports > ch)
> >	    jack_connect (self->jack_client, jack_port_name 
> >	    (jack->output_ports[ch]), details.input_port_names[ch].c_str());
> >	}
> >   }
> 
> you're not setting error in case you failed to find
> the required devices to connect the channels.

I don't want to. If you for instance start beast with

-pjack=jamin

to use jamin for mastering, and later the user chooses to terminate
jamin, then we can still run. Its just that then the ports will be
initially unconnected, which is not a problem, because it can be easily
fixed in qjackctl.

> > /* setup buffer size */
> >
> > // keep at least two jack callback sizes for dropout free audio
> > guint min_buffer_frames = jack_get_buffer_size (self->jack_client) * 2;
> >
> > // keep an extra engine buffer size (this compensates also for cases 
> > where the
> > // engine buffer size is not a 2^N value, which would otherwise cause the
> 
> yeah, engine buffers are usually not 2^n sizes. see bse_engine_constrain()
> in bseengine.c.
> 
> > // buffer never to be fully filled with 2 periods of data)
> 
> "periods"? do you mean "elements" or "frames" here? (using your ringbuffer
> terminology)
> or do you mean buffer sizes here? jack buffers or bse buffers?

Jack buffer sizes.

> 
> > min_buffer_frames += BSE_PCM_DEVICE (device)->req_block_length;
> 
> couldn't you instead just align min_buffer_frames to 2 * bse buffer size,
> to still fullfil the requirements you outlined, and yet yield a smaller
> ring buffer (thus optimize latency)?

Don't know. Trouble comes when we miss the - racy - condition. That
produces one period of extra latency if things go badly wrong. Thus a
stateful synchronization which can't miss something (such as a pipe())
would be superior. Then I suppose we could do as you suggested.

> > // honor the user defined latency specification
> > //
> > // FIXME: should we try to tweak local buffering so that it corresponds
> > // to the user defined latency, or global buffering (including the jack
> > // buffer)? we do the former, since it is easier
> 
> we should do the latter. that shouldn't be too hard, Paul said that you
> could query (and inform) jack about the latency on a port.
> that then just needs to be subtracted from BSE_PCM_DEVICE 
> (device)->req_latency_ms.

It can change. If the beast output is jamin, that'd be adding latency.
If the user reconnects jamin to a convolution which adds latency, then
the total output latency query would change while we're running. Thats
why I am a bit reluctant to do it, as its results are not well-defined.

It could even happen that we are not connected initially (latency = 0),
and only get connected afterwards by a patchbay, where the user is
loading a session. Thus in such a case we would always misbehave.

> 
> > guint user_buffer_frames = BSE_PCM_DEVICE (device)->req_latency_ms * 
> > handle->mix_freq / 1000;
> > guint buffer_frames = max (min_buffer_frames, user_buffer_frames);
> >
> > jack->input_ringbuffer.resize (buffer_frames, handle->n_channels);
> > jack->output_ringbuffer.resize (buffer_frames, handle->n_channels);
> 
> the way this is written, you're doubling the latency because you apply
> it to the input and the output buffer. you should just apply it to the
> output buffer (and also subtract the input buffer latency before doing
> that)

The input buffer should not produce latency. Ideally, the filled frames
of the input buffer and the filled frames of the output buffer should
add up to exactly buffer_frames. Note that the input_buffer is *empty*
initially while the output_buffer is *full* initially, and the jack
callback methodically only adds as many frames to the input_buffer as it
takes from the output_buffer, whereas the engine process cycle does the
inverse.

> > jack->buffer_frames  = jack->output_ringbuffer.get_writable_frames();
> 
> what is jack->buffer_frames good for? and why is that different from
> total_n_frames() here?

Its not different. Don't know. Maybe I'll get rid of it in the new
version.

> > // the ringbuffer should be exactly as big as requested
> > g_assert (jack->buffer_frames == buffer_frames);
> > DEBUG ("ringbuffer size = %.3fms", jack->buffer_frames / double 
> > (handle->mix_freq) * 1000);
> 
> hm, you should better use driver prefixing in DEBUG() statements like
> the ALSA driver does it, i.e.:
>    DEBUG ("JACK: ringbuffer size = %.3fms", jack->buffer_frames / double 
>    (handle->mix_freq) * 1000);
> of course that applies to all your DEBUG() calls.

I think the DEBUG() automagically does this. At least I saw that the
output contained JACK already, so I didn't bother to add it.

> > /* setup PCM handle or shutdown */
> > if (!error)
> >   {
> >     bse_device_set_opened (device, dname, handle->readable, 
> >     handle->writable);
> >     if (handle->readable)
> >       handle->read = jack_device_read;
> >     if (handle->writable)
> >       handle->write = jack_device_write;
> >     handle->check_io = jack_device_check_io;
> >     handle->latency = jack_device_latency;
> >     BSE_PCM_DEVICE (device)->handle = handle;
> >
> >     /* will prevent dropouts at initialization, when no data is there at 
> >     all */
> 
> why would this "prevent dropouts"? the jack thread got started already,
> so a dropout could possibly have occoured _already_.

No the jack callback is still operating in the "inactive" state, so it
will not touch our data yet. Yes. It will write zeros. And no. Normally
you will not hear this.

The point here is to fill the output ringbuffer fully with zeros (the
alsa driver does the same), so that beast has some time to produce some
audio that will be played. Otherwise it could be that there are dropouts
at song start because jack is consuming the data about as fast at beast
can produce it initially.

> granted, retriggering may be neccessary, but the comment doesn't make
> sense to me here.
> 
> >     jack_device_retrigger (jack);
> >     jack_device_latency (handle);   // debugging only: print latency 
> >     values
> >   }
> > else
> >   {
> >     terminate_and_free_jack (jack);
> >   }
> > DEBUG ("JACK: opening PCM \"%s\" readupble=%d writable=%d: %s", dname, 
> > require_readable, require_writable, bse_error_blurb (error));
> >
> > return error;
> >}
> >
> >static void
> >bse_pcm_device_jack_close (BseDevice *device)
> >{
> > JackPcmHandle *jack = (JackPcmHandle*) BSE_PCM_DEVICE (device)->handle;
> > BSE_PCM_DEVICE (device)->handle = NULL;
> >
> > terminate_and_free_jack (jack);
> >}
> >
> >static void
> >bse_pcm_device_jack_finalize (GObject *object)
> >{
> > BsePcmDeviceJACK *self = BSE_PCM_DEVICE_JACK (object);
> > if (self->jack_client)
> >   {
> >     jack_client_close (self->jack_client);
> >     self->jack_client = NULL;
> >   }
> >
> > /* chain parent class' handler */
> > G_OBJECT_CLASS (parent_class)->finalize (object);
> >}
> >
> >static void
> >jack_device_retrigger (JackPcmHandle *jack)
> >{
> > g_return_if_fail (jack->jack_client != NULL);
> >
> > /*
> 
> extraneous newline at coment start.
> 
> >  * we need to reset the active flag to false here, as we modify the
> >  * buffers in a non threadsafe way; this is why we also wait for
> >  * the condition, to ensure that the jack callback really isn't
> >  * active any more
> >  */
> > Atomic::int_set (&jack->callback_state, CALLBACK_STATE_INACTIVE);
> >
> > /* usually should not timeout, but be notified by the jack callback */
> > jack->cond_mutex.lock();
> > if (!jack->is_down)
> >   jack->cond.wait_timed (jack->cond_mutex, 100000);
> > jack->cond_mutex.unlock();
> 
> hm, is_down is set by the jack thread onkly volountarily, i.e. if
> it exits prematurely, you get the stale BSE thread i mntioned above.

Nope. Its set by the shutdown handler.

> 
> >
> > /* jack_ringbuffer_reset is not threadsafe! */
> 
> yeah, i didn't mention this in the ringbuffer review, but
> i'd agree that "reset" would be a better name than "clear"
> for the resetting of the pointers.

Although the comment refers back to the old days, where I used jacks
ringbuffer. Thus it also is called jack_ringbuffer_reset, and not
FrameRingBuffer<float>::clear() in the comment.

> >static gsize
> >jack_device_read (BsePcmHandle *handle,
> >                 gfloat       *values)
> >{
> [...]
> 
> >
> >	  if (jack->is_down)
> >	    {
> >	      /*
> 
> extraneous newline at comment start.
> 
> >	       * FIXME: we need a way to indicate an error here; beast 
> >	       should provide
> >	       * an adequate reaction in case the JACK server is down (it 
> >	       should stop
> >	       * playing the file, and show a dialog, if JACK can not be 
> >	       reconnected)
> 
> we don't need error indication here (see alsa_device_read()). it's
> good enough if we return gracefully here.
> error checking code (and returns) can be executed by check_io()
> and/or retrigger().

Yes, we do. If JACK dies, I want to know as user. Otherwise I'll just
see that nothing happens. Why? And why is beast continuing to play if
still nothing happens. Why should it...?

> >	       *
> >	       * if we have a way to abort processing, this if can be moved 
> >	       above
> >	       * the condition wait; however, right now moving it there 
> >	       means that
> >	       * beast will render the output as fast as possible when jack 
> >	       dies, and
> >	       * this will look like a machine lockup
> 
> no, endless loops don't look like lockups ;)
> in the first case, your CPU meter runs at 100% and toggling Num-lock/etc.
> keys still work (usually also the X server etc.) while in the latter case
> *nothing* goes anymore.

But it isn't a situation I would want to throw the user into either.

> >	       */
> >	      Block::fill (frames_left * handle->n_channels, values, 0.0);   
> >	      /* <- remove me once we can indicate errors */
> 
> nope should stay (the same way alsa_device_read() copeswith errors).

Its uncommon for a hardware device to go away while the application
runs. Its unfortunately common for the jack server to go away. That
might not be sloppy programming. The jack server might as well go away
intentionally because we missed a deadline. For instance because the
user was running jack and beast without rt prio enabled but had a
semi-low latency. Just stopping in such cases is rather uninformative,
and people may think its a beast bug or something.

We should be able to present a reason why this happened to the user
inside a nice dialog and then stop trying to play audio. Its pointless
after jackd terminated our connection or terminated itself or crashed or
something.

> > const gchar *info = g_intern_printf (/* TRANSLATORS: keep this text to 70 
> > chars in width */
> >                                      _("JACK Audio Connection Kit PCM 
> >                                      driver (http://jackaudio.org)\n"));
> 
> jack version printing? again, see what the alsa driver does here.

There is no jack version I can ask for in the API, so I can't print one.
I could print the version of libjack I was compiled against (i.e.
something that pkg-config produced or so), but the server version might
be different, as long as the protocol version of the server and the
protocol version of libjack match.

> the single most important issue that has to be fixed here is
> the mutex and condition. neither the BSE nor the JACK thread
> can afford acquiring it and wait for the other thread.
> that's why we need a ringbuffer based only on atomic ops. we
> got that now, so the next step is to modify the code to use
> it up to its fullest potential.
> 
> basically, to get there, the jack thread needs to be modified
> to act more similarl to e.g. the kernel ALSA driver wrg to
> xruns. the kernel driver also has fixed-size ring buffers and
> still deals with drop outs perfectly well. that kind of API
> semantic has to be provided for the BSE thread in the bse-alsa
> driver. then, the implementations of check_io(), retrigger(),
> read() and write() can be much closer to the bse-alsa driver
> implementation. also, this will eliminate the need for the
> mutex and the condition.

I think I don't know enough yet to give a qualified answer on what to
do: especially I want to know, if

 * write to pipe
 * pthread_condition_signal
 * (maybe semaphores)
 * (maybe something else I forgot)

are equally capable in their realtimeness properties, and suitable for
notifying the bse master thread from the jack realtime thread, and so
which is the one to choose to let the bse thread know when to compute
date.

Timeouts are a bad solution because they either occur too soon (in which
case the engine thread may wake up, but can't do the computation), or
too late, because there is always jitter. In any case you loose precious
time or waste precious CPU cycles or both.

I know that for practical purposes, there is no other solution the bse
framework is capable than write-to-pipe, but I have seen in kernel code
that the implentation is not lock-free. Can this lead to priority
inversion? Can conditions do the same? What is safe and what is not?
Right now, I don't know.

   Cu... Stefan
-- 
Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan