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

[Stefan Westerfeld <stefan space twc de>]

   Hi!

On Mon, Jun 05, 2006 at 04:31:23AM +0200, Tim Janik wrote:
> >>>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.
> 
> ok. note that beast doesn't 100% perfectly prevent priority inversion in
> other scenarios either. this issue is slowly being worked on (the idea
> is to move all communication to atomic ops guarded by BirnetGuard, but
> that's an entire new thread ;)

However, these are two kinds of separate issues:
(1) priority inversion occuring within the jack realtime thread
(2) priority inversion occuring elsewhere in beast

When (1) is being triggered, the jackd server can get dropouts while
writing audio, and might decide that we are the client that is badly
implemented. So it might shutdown the connection we have, leaving the
other clients running.

When (2) is being triggered, we miss the opportunity to refill the
ringbuffer in time, so there will be a click in the output, but at least
the audio processing still continues; but we also may have still enough
data buffered in the rinbuffer, and nothing will happen; that will also
depend on how the user configured the beast latency.

So I am trying harder to avoid (1) than (2). If beast would not be
likely to produce more underruns in (2) than in (1), we might as well
run the beast processing within the jack RT thread. But we don't and
thats good as it is.

I still agree with your concluding remarks, though, which are:

> until then, the jack RT thread should use a pipe write() to notify the
> engine and sequencer threads, which both create their own pipe fds that
> they also poll() on.

After thinking some more about it, and also reading Paul's oppinion:

  http://permalink.gmane.org/gmane.linux.audio.devel/11725

I think that pipe write() is probably ok for a solution running under
linux. Its simply that linux is no realtime operating system, and so
whatever we do in our case, we can get no guarantee that it will work in
100% of the cases. So if this was an realtime airplane control software,
we should probably use some other operating systen.

However, for an audio software its probably enough if it works reliable
enough for daily music production, and it pipe writes should deliver
this degree of reliability.

> >>>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.
> 
> fine, then we need to roll our own function that does this.
> for existing example code that stringifies foreign errors,
> you can take a look at gslvorbis-cutter.c:ov_error_blurb().
> 
> it might be agood idea to submit this to jack once done btw.

Its not that writing our own function will help us a lot though. Its not
that every function returns a jack_status_t. Just the open function.

The usual way for getting stringified errors in other situations is to
set the error callback with jack_set_error_function.

> >>>	    }
> >>>	}
> >>>    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.
> 
> well, it's not exactly "hard to optimize" here. just return a pointer.

I am trying to avoid manual new/delete in all code I write, because its
a potential source of errors. So returning a pointer is not so good. It
also makes map accesses somewhat strange to read, i.e.

  (*devices)["foo"] = bar;

instead of

  devices["foo"] = bar;

Under some circumstances, the C++ compiler will also construct objects
that are returned from a function within the memory of the caller, so
that no copy will done at all. But I don't know if the device map case
is one of these cases, or not.

So if you insist that it must be "optimized", I would suggest changing
the API to

static void
query_jack_devices (BsePcmDeviceJack           *self,
                    map<string, DeviceDetails> &devices)

then I can still keep my automatic deletion feature and nice to read
map access code.

> >>>     * 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.
> 
> the rationale provided here:
>   http://mail.gnome.org/archives/beast/2006-June/msg00005.html
> for why writes shouldn't block applies also to the bse-alsa driver.
> the fact that bse-alsa could indeed block in its write function
> since we set the device to nonoblock=0 is merely the guarding
> i'm talking about at the end of the above email.

Ok, investigating the question pointed out to in the mail you linked
there is still on my todo.

> >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.
> 
> well, forget about priority inversion for the moment. it's very hard
> to trigger anyway. a much stronger reason to use a pipe is that the
> engine and synthesizer threads use poll() and not cond_wait.

Yes, I changed my mind. I will use write().

> >>>/* 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.
> 
> "So what?" ? there's no point in connectiong ports if the
> devices can't be found.

I will not "connect" the points if the device can not be found, since
then the (di != devices.end()) condition (4 lines below from here) will
be false. However, I still need to register the beast output ports, so
that beast can write output, although then it will be unconnected when
writing output, which is perfectly valid in the jack world. It happens
all the time. The user can just connect it afterwards using a patchbay.

> >>>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.
> 
> sorry, would you care to enlighten me here please?
> what's -pjack=jamin supposed to mean?
> should the beast output ports be auto-connected to input ports named 
> "jamin"?
Yes.

> and, if input ports named "jamin" are not present, that'll silently be
> ignored?
Yes.

> will the jack callback thread still properly run and request BSE engine data
> if the beast output port is unconnected?
Yes.

> if that's the case, i think at least the behaviour of silently ignoring
> unsuccessfull connections has to be changed. we also throw proper errors
> in other drivers if unconectable devices are being adressed.
> (if unsuccessful connections really are common usage with jack, we can
> still add device flags, like we do with OSS, so you could say 
> -pjack=jamin,auto for automated connections)

I don't know from your example what you envision the syntax to be like.
There are four good cases as I see the situation.

(1) request beast to autoconnect, and request beast to find a suitable
    device to autoconnect to without asking

-> this is what -pjack does right now, and I think it should be kept
   this way

(2) request beast to connect to a specific client if possible, but
    continue if that doesn't work (we could have some [X] report errors
    on jack connection problems message, then the user could disable
    the error, or we could make this configurable)

-> this is what -pjack=jamin does right now

(3) request beast to connect to a specific client, and let it fail and
    report an error if it doesn't work

(4) request beast to start without connecting a client

-> this is what -pjack=unconnected does right now, as long as there is
   no jack client called "unconnected", in which case this behaves like
   (2)

What I wasn't too sure about when designing the syntax is whether the
position of the argument should play a role. Suppose we also decided
some day to add ro (readonly) and wo (writeonly) as options. Should then

-p jack=auto,ro,alsa_pcm

be the same like

-p jack=alsa_pcm,ro,auto

This rules out the possibility of connecting clients called "auto" or
"ro", which is maybe a bad decision. So maybe we should start with
something like

-p jack=connect:alsa_pcm                          
-p jack=connect:alsa_pcm,wo

or so? I am really not sure what would be a good and clean syntax here,
that handles all cases that are needed.

> >>>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.
> 
> sorry? what do you mean by "stateful synchronization"?

By stateful synchronization I mean that writing to a pipe will
definitely wakeup the engine thread later, whereas signalling a
condition will only wakeup the engine thread if it has been sleeping on
the condition. The former is better.

> >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.
> 
> what's that about being non-connected.
> does jack still request audio from beast when unconnected?

Yes.

> and, you said jack clients would always auto connect to alsa_pcm
> and that alsa_pcm should always be present. so why would we start
> up disconnected?

Thats not what I wanted to say. I just pointed out that lots of people
hardcode alsa_pcm somewhere in their code. However, whether a jack
application should startup connected or unconnected, or how defaulting
or auto connecting should work for "normal" jack applications is an open
issue as far as I understand from reading the mailing list. There have
been some threads suggesting this or that (like api extensions, extra
default flags on ports and the like), and some people have been pointing
out that they like this or that behaviour better.

So I think until people have figured out what they want, I'd like to see
beast to support both: connect on open and open unconnected (so that
some external program can do the work).

> >>>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.
> 
> that means input latency increases with every output underrun, because
> in that case you're faking extra output values. basically, in duplex
> scenrios under latency constraints you have to properly resync input
> and output and can't just pad output values.

I don't think it can happen. After an output underrun, the
jack_device_retrigger code clears both ringbuffers (input and output),
and then refills the output buffer with zeros.

> >>>	       * 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...?
> 
> i already said that. but i can say it again if that helps you.
> you should gracefully return and ignore the error in write().
> you can check for error conditions in check_io() and there we
> can decide and undertake appropriate measures to deal with the
> error. e.g. by resyncing the stream (that's at least interesting
> for ALSA/OSS or by notifying the user, etc.)

Yes, right. If I implement check_io properly, one of the problems goes
away.

> >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.
> 
> note that the BSE audio driver model *and* the BSE engine model are based
> on timeouts. that's what check_io() provides and what the engine needs.
> you won't get any other programming model with BSE, regardless of whether
> you like it or not ;)

Well, pipes are another programming model. I don't think we'll need
timeouts when we can use pipes. But right now I can only make
theoretical claims about it: I will have to work on the code to see
whether the code can be written in a way so that check_io will rely on
the notifications of pipes (and the lockless ringbuffers) only. Then it
will be able to return an arbitary timeout, but the pipe will still
ensure that everything goes well.

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