bsepcmdevice-jack.cc (Re: Beast Jack driver)
- From: Tim Janik <timj gtk org>
- To: Stefan Westerfeld <stefan space twc de>
- Cc: beast gnome org, paul linuxaudiosystems com
- Subject: bsepcmdevice-jack.cc (Re: Beast Jack driver)
- Date: Fri, 2 Jun 2006 01:45:01 +0200 (CEST)
On Thu, 1 Jun 2006, Stefan Westerfeld wrote:
Anyway, here is the C++ source (the interesting part, for the review):
/* BSE - Bedevilled Sound Engine
* Copyright (C) 2004 Tim Janik
* Copyright (C) 2006 Stefan Westerfeld
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 2 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General
* Public License along with this program; if not, write to the
* Free Software Foundation, Inc., 59 Temple Place, Suite 330,
* Boston, MA 02111-1307, USA.
*/
#include "configure.h"
#include "bsepcmdevice-jack.hh"
#include <bse/bseblockutils.hh>
#include <bse/gsldatautils.h>
#include <jack/jack.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <set>
#include <string>
#include <map>
#include <vector>
using namespace Birnet;
using std::vector;
using std::set;
using std::map;
using std::string;
using std::min;
using std::max;
using std::copy;
using Bse::Block;
namespace {
/**
* This function uses std::copy to copy the n_values of data from ivalues
* to ovalues. If a specialized version is available in bseblockutils,
* then this - usually faster - version will be used.
well, you could even say "suppsoed to be faster", because if the blockutils
version is indeed slower, that's worthy of a bug report.
*
* The data in ivalues and ovalues may not overlap.
*/
template<class Data> void
fast_copy (guint n_values,
Data *ovalues,
const Data *ivalues)
{
copy (ivalues, ivalues + n_values, ovalues);
}
template<> void
fast_copy (guint n_values,
float *ovalues,
const float *ivalues)
{
Block::copy (n_values, ovalues, ivalues);
}
template<> void
fast_copy (guint n_values,
guint32 *ovalues,
const guint32 *ivalues)
{
Block::copy (n_values, ovalues, ivalues);
}
/**
* The FrameRingBuffer class implements a ringbuffer for the communication
[...]
[ring buffer code that has already been reviewed...]
the ring buffer code is probably best split off into an extra tcc file,
especially if chances are that you need it in the MIDI driver as well.
static BIRNET_MSG_TYPE_DEFINE (debug_pcm, "pcm", BIRNET_MSG_DEBUG, NULL);
#define DEBUG(...) sfi_debug (debug_pcm, __VA_ARGS__)
/* --- JACK PCM handle --- */
there should be an explanatory comment in this file on the jack thread
and when it enters what states, etc.
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.
struct JackPcmHandle
{
BsePcmHandle handle;
JackPcmHandle (jack_client_t *jack_client)
: jack_client (jack_client)
cosmetic, the ':' usually goes next to the ')', i.e.:
JackPcmHandle (jack_client_t *jack_client) :
jack_client (jack_client)
{}
{
memset (&handle, 0, sizeof (handle));
callback_state = ixruns = pixruns = oxruns = poxruns = 0;
is_down = false;
hm, afaik, it's better style to intiialize all these
after JackPcmHandle() :
}
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)
jack_client_t *jack_client;
FrameRingBuffer<float> input_ringbuffer;
FrameRingBuffer<float> output_ringbuffer;
int callback_state;
^^^^^^^^^^
proper typing is missing here, you may take a look at bsemidireceiver.cc and
search for "enum" to find examples for internally used enums.
int ixruns;
int oxruns;
int pixruns;
int poxruns;
bool is_down;
};
}
/* --- prototypes --- */
static void bse_pcm_device_jack_class_init (BsePcmDeviceJACKClass *klass);
static void bse_pcm_device_jack_init (BsePcmDeviceJACK *self);
static gsize jack_device_read (BsePcmHandle *handle,
gfloat *values);
static void jack_device_write (BsePcmHandle *handle,
const gfloat *values);
static gboolean jack_device_check_io (BsePcmHandle *handle,
glong *tiumeoutp);
static guint jack_device_latency (BsePcmHandle *handle);
static void jack_device_retrigger (JackPcmHandle *jack);
/* --- define object type and export to BSE --- */
static const char type_blurb[] = ("PCM driver implementation for JACK Audio Connection Kit "
"(http://jackaudio.org)");
BSE_REGISTER_OBJECT (BsePcmDeviceJACK, BsePcmDevice, NULL, "", type_blurb, NULL, bse_pcm_device_jack_class_init, NULL, bse_pcm_device_jack_init);
hm, i guess this could be nicer as BsePcmDeviceJack, espcially for the class
name BsePcmDeviceJackClass. allthoguh JACK is often upper cased like the
abbreviation BSE, we also lower case it to Bse in namespaces for readability.
and the http://jackaudio.org/ also has a few examples of "Jack" ;)
BSE_DEFINE_EXPORTS (__FILE__);
/* --- variables --- */
static gpointer parent_class = NULL;
hm, i should fix BSE_REGISTER_OBJECT() to act like G_DEFINE_TYPE and
provide a parent_class for you.
/* --- functions --- */
static void
bse_pcm_device_jack_init (BsePcmDeviceJACK *self)
{
/* FIXME: move signal handling somewhere else */
needs to be moved into birnetthread.c in particular.
sigset_t signal_mask;
sigemptyset (&signal_mask);
sigaddset (&signal_mask, SIGPIPE);
int rc = pthread_sigmask (SIG_BLOCK, &signal_mask, NULL);
if (rc != 0)
g_printerr ("jack driver: pthread_sigmask for SIGPIPE failed: %s\n", strerror (errno));
this is what you'd usually use g_warning() for, since it's unexpected.
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?
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);
?
* FIXME: what about server name? (necessary if more than one jackd instance is running)
* FIXME: get rid of device unloading, so that the jackd connection can be kept initialized
altering the device unloading will have to be done differently,
the attempt to connect to the jack server will have to be made
much earlier in that case.
(in other words, i don't think you need this FIXME in this place,
altering the driver loading + startup logic will be an entirely
seperate undertaking)
*
* maybe move this to a different function
that can be done once needed.
*/
}
namespace {
struct DeviceDetails {
guint ports, input_ports, output_ports, physical_ports, terminal_ports;
vector<string> input_port_names;
vector<string> output_port_names;
DeviceDetails() : ports (0), input_ports (0), output_ports (0), physical_ports (0), terminal_ports (0)
please put a newline after the ':'
{
}
};
}
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?
* - jackd is not running
that should/will actually be determined by jack_client_open(), right?
*/
const char **jack_ports = self->jack_client ? jack_get_ports (self->jack_client, NULL, NULL, 0) : NULL;
if (jack_ports)
{
for (guint i = 0; jack_ports[i]; i++)
{
const char *end = strchr (jack_ports[i], ':');
if (end)
{
string device_name (jack_ports[i], end);
hm, please don't do "using std::string;" (further above)
but instead use std::string or just String which is provided
by using Birnet.
DeviceDetails &details = devices[device_name];
details.ports++;
why are you incrementing here already, if the port
could possibly not be looked up by name?
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...
}
}
}
free (jack_ports);
}
return devices;
ugh, isn't this a full fledged recursive map+strings+DeviceDetails copy?
}
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? ;)
/* 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?
if (device_name == "alsa_pcm")
ring = sfi_ring_prepend (ring, entry);
else
ring = sfi_ring_append (ring, entry);
}
if (!ring)
ring = sfi_ring_append (ring, bse_device_error_new (device, g_strdup_printf ("No devices found")));
return ring;
}
static void
jack_shutdown_callback (void *jack_handle_ptr)
{
JackPcmHandle *jack = static_cast <JackPcmHandle *> (jack_handle_ptr);
AutoLocker cond_locker (jack->cond_mutex);
jack->is_down = true;
jack->cond.signal();
}
static int
this misses a comment on the purpose of the return value.
jack_process_callback (jack_nframes_t n_frames,
this, along with most other functions, should have a comment
that says which thread executes the function. e.g. like the
/* UserThread */ and /* EngineThread */ comments in bsepcmmodule.c
(or possibly even more elaborate)
void *jack_handle_ptr)
{
typedef jack_default_audio_sample_t sample_t;
we use mixed case typenames, so this should be:
typedef jack_default_audio_sample_t Sample;
or:
typedef jack_default_audio_sample_t SampleType;
JackPcmHandle *jack = static_cast <JackPcmHandle *> (jack_handle_ptr);
int callback_state = Atomic::int_get (&jack->callback_state);
hm, callback_state is not declared as volatile above...
we had this in the ringbuffer already.
/* still waiting for initialization to complete */
if (callback_state == CALLBACK_STATE_ACTIVE)
{
bool have_cond_lock = jack->cond_mutex.trylock();
guint n_channels = jack->handle.n_channels;
float buffer[n_frames * n_channels];
if (jack->handle.readable)
{
/* interleave input data for processing in the engine thread */
g_assert (jack->input_ports.size() == n_channels);
for (guint ch = 0; ch < n_channels; ch++)
{
sample_t *values = (sample_t *) jack_port_get_buffer (jack->input_ports[ch], n_frames);
float *p = &buffer[ch];
for (guint i = 0; i < n_frames; i++)
{
*p = *values++;
p += n_channels;
}
}
guint frames_written = jack->input_ringbuffer.write (n_frames, buffer);
if (frames_written != n_frames)
Atomic::int_add (&jack->ixruns, 1); /* input underrun detected */
}
if (jack->handle.writable)
{
guint read_frames = jack->output_ringbuffer.read (n_frames, buffer);
if (read_frames != n_frames)
{
Atomic::int_add (&jack->oxruns, 1); /* output underrun detected */
was declared non-volatile oxruns.
Block::fill ((n_frames - read_frames) * n_channels, &buffer[read_frames * n_channels], 0.0);
}
/* deinterleave output data for processing in the engine thread */
g_assert (jack->output_ports.size() == n_channels);
for (guint ch = 0; ch < n_channels; ch++)
{
sample_t *values = (sample_t *) jack_port_get_buffer (jack->output_ports[ch], n_frames);
sample_t *values_end = values + n_frames;
float *p = &buffer[ch];
while (values < values_end)
{
*values++ = *p;
p += n_channels;
}
}
}
/*
commanets shouldn't start with an empty line unless its a comment
for the documentation system.
* as we can't (always) lock our data structures from the jack realtime
* thread, using a condition introduces the possibility for a race:
this type of comment should better be reworded in a general comment on how
things are supposed to work and communicate. then it can go before the
declaration of BsePcmDeviceJACK.
*
* 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.
* [jack thread] reads some data from output_ringbuffer
* [jack thread] signals the condition
* [bse thread] continues to write some data to output_ringbuffer
basically, the above can work if you s/sleeps on the condition/continues
with audio processing/. BSE can do that, once it has the required timeout
information.
*
* race condition:
*
* [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
*
* 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.
*/
if (have_cond_lock)
jack->cond_mutex.unlock();
}
else
{
when exactly is this branch tirggered? (lost track due to the excessive
inlined commenting above.)
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.
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...
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.
}
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?
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;
}
static BseErrorType
bse_pcm_device_jack_open (BseDevice *device,
gboolean require_readable,
gboolean require_writable,
guint n_args,
const gchar **args)
{
BsePcmDeviceJACK *self = BSE_PCM_DEVICE_JACK (device);
if (!self->jack_client)
return BSE_ERROR_FILE_OPEN_FAILED;
JackPcmHandle *jack = new JackPcmHandle (self->jack_client);
BsePcmHandle *handle = &jack->handle;
handle->readable = require_readable;
handle->writable = require_writable;
handle->n_channels = BSE_PCM_DEVICE (device)->req_n_channels;
/* try setup */
BseErrorType error = BSE_ERROR_NONE;
const char *dname = (n_args == 1) ? args[0] : "alsa_pcm";
handle->mix_freq = jack_get_sample_rate (self->jack_client);
Atomic::int_set (&jack->callback_state, CALLBACK_STATE_INACTIVE);
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).
if (port)
jack->input_ports.push_back (port);
else
error = BSE_ERROR_FILE_OPEN_FAILED;
snprintf (port_name, port_name_size, "out_%u", i);
port = jack_port_register (self->jack_client, port_name, JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0);
if (port)
jack->output_ports.push_back (port);
else
error = BSE_ERROR_FILE_OPEN_FAILED;
}
/* 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)?
/* connect ports */
map<string, DeviceDetails> devices = query_jack_devices (self);
querying the devices could have been done before registering
the ports above.
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.
/* 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?
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)?
// 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.
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)
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?
// 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.
/* 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_.
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.
/* 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.
jack->input_ringbuffer.clear();
jack->output_ringbuffer.clear();
/* initialize output ringbuffer with silence */
vector<float> silence (jack->buffer_frames * jack->handle.n_channels);
this is expensive, you can simply use bse_engine_const_zeros() which
provides at least BSE_STREAM_MAX_VALUES many values in constant time.
and yes, this should be fixed in the ALSA driver as well.
guint frames_written = jack->output_ringbuffer.write (jack->buffer_frames, &silence[0]);
g_assert (frames_written == jack->buffer_frames);
DEBUG ("jack_device_retrigger: %d frames written", frames_written);
hm, at the end of retrigger, the jack thread should go ACTIVE.
just like we call snd_pcm_prepare() in bsepcmdevice-alsa.c:alsa_device_retrigger().
}
static gboolean
jack_device_check_io (BsePcmHandle *handle,
glong *timeoutp)
{
JackPcmHandle *jack = (JackPcmHandle*) handle;
g_return_val_if_fail (jack->jack_client != NULL, false);
/*
int ixruns = Atomic::int_get (&jack->ixruns);
int oxruns = Atomic::int_get (&jack->oxruns);
if (jack->poxruns != oxruns || jack->pixruns != ixruns)
{
printf ("beast caused jack input xruns %d, output xruns %d\n", ixruns, oxruns);
debugging prints (if you already failed to use DEBUG()) should go to
stderr, otherwise they cause major breakage in programs that generate
data on stdout (of which we have quite bunch in CVS).
jack->poxruns = oxruns;
jack->pixruns = ixruns;
jack_device_retrigger (jack);
}
*/
/* (FIXME?)
*
* we can not guarantee that beast will read the data from the jack ringbuffer,
* since this depends on the presence of a module that actually reads data in
* the synthesis graph : so we simply ignore the ixruns variable; I am not sure
* if thats the right thing to do, though
sounds reasonably at first glance.
*/
int oxruns = Atomic::int_get (&jack->oxruns);
if (jack->poxruns != oxruns)
{
g_printerr ("%d beast jack driver xruns\n", oxruns);
jack->poxruns = oxruns;
jack_device_retrigger (jack);
you shouldn't need to retrigger after xruns have occoured.
it has already happened and extra padding was provided already.
if you do this, you just make sure that there really is a
loud and noticable click in the output (while a simply
1-block xrun often doesn't produce too bad artefacts)
}
Atomic::int_set (&jack->callback_state, CALLBACK_STATE_ACTIVE);
this should have gone into retrigger, see alsa_device_retrigger() and
alsa_device_check_io().
guint n_frames_avail = min (jack->output_ringbuffer.get_writable_frames(), jack->input_ringbuffer.get_readable_frames());
/* check whether data can be processed */
if (n_frames_avail >= handle->block_length)
return TRUE; /* need processing */
/* calculate timeout until processing is possible or needed */
guint diff_frames = handle->block_length - n_frames_avail;
*timeoutp = diff_frames * 1000 / handle->mix_freq;
return FALSE;
}
static guint
jack_device_latency (BsePcmHandle *handle)
{
JackPcmHandle *jack = (JackPcmHandle*) handle;
jack_nframes_t rlatency = 0, wlatency = 0;
g_return_val_if_fail (jack->jack_client != NULL, 0);
/* FIXME: the API of this function is broken, because you can't use its result
* to sync for instance the play position pointer with the screen
*
* why? because it doesn't return you rlatency and wlatency seperately, but
* only the sum of both
*
* when using jack, there is no guarantee that rlatency and wlatency are equal
good point.
and more importantly, for song pointer syncs, we want to use just the
wlatency. so for a first fix, we can change oss_device_latency() and
alsa_device_latency() to just return the wlatency.
we can later improve the API to also support rlatency queries.
*/
for (guint i = 0; i < jack->input_ports.size(); i++)
{
jack_nframes_t latency = jack_port_get_total_latency (jack->jack_client, jack->input_ports[i]);
rlatency = max (rlatency, latency);
}
for (guint i = 0; i < jack->output_ports.size(); i++)
{
jack_nframes_t latency = jack_port_get_total_latency (jack->jack_client, jack->output_ports[i]);
wlatency = max (wlatency, latency);
}
guint total_latency = 2 * jack->buffer_frames + rlatency + wlatency;
ah, this is where buffer_frames gets used?
can't you simply use jack->input_ringbuffer.n_total_frames() +
jack->output_ringbuffer.n_total_frames().
(also be more accurate if the buffers have different lengths,
e.g. the input buffer could possibly be shorter than the output
buffer).
DEBUG ("rlatency=%.3f ms wlatency=%.3f ms ringbuffer=%.3f ms total_latency=%.3f ms",
rlatency / double (handle->mix_freq) * 1000,
wlatency / double (handle->mix_freq) * 1000,
jack->buffer_frames / double (handle->mix_freq) * 1000,
total_latency / double (handle->mix_freq) * 1000);
return total_latency;
}
static gsize
jack_device_read (BsePcmHandle *handle,
gfloat *values)
{
JackPcmHandle *jack = (JackPcmHandle*) handle;
g_return_val_if_fail (jack->jack_client != NULL, 0);
g_return_val_if_fail (Atomic::int_get (&jack->callback_state) == CALLBACK_STATE_PACTIVE, 0);
guint frames_left = handle->block_length;
while (frames_left)
{
guint frames_read = jack->input_ringbuffer.read (frames_left, values);
frames_left -= frames_read;
values += frames_read * handle->n_channels;
if (frames_left)
{
AutoLocker cond_locker (jack->cond_mutex);
/* usually should not timeout, but be notified by the jack callback */
if (!jack->input_ringbuffer.get_readable_frames())
jack->cond.wait_timed (jack->cond_mutex, 100000);
we can't afford to wait on jack here. engine locks have to be
super-short (examples can be found in bseengineutils.c).
i said this above already, we can't actually afford locking at all,
which is why we have an atomic ringbuffer in the first place.
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().
*
* 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.
*/
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).
return handle->block_length * handle->n_channels;
}
}
}
return handle->block_length * handle->n_channels;
}
static void
jack_device_write (BsePcmHandle *handle,
const gfloat *values)
{
JackPcmHandle *jack = (JackPcmHandle*) handle;
g_return_if_fail (jack->jack_client != NULL);
g_return_if_fail (Atomic::int_get (&jack->callback_state) == CALLBACK_STATE_ACTIVE);
guint frames_left = handle->block_length;
while (frames_left)
{
guint frames_written = jack->output_ringbuffer.write (frames_left, values);
frames_left -= frames_written;
values += frames_written * handle->n_channels;
if (frames_left)
{
AutoLocker cond_locker (jack->cond_mutex);
/* usually should not timeout, but be notified by the jack callback */
if (!jack->output_ringbuffer.get_writable_frames())
jack->cond.wait_timed (jack->cond_mutex, 100000);
if (jack->is_down)
{
/*
* 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)
*
* 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
*/
hrm, /* FIXME: see jack_device_read() */ would have been easier here.
my comments there also apply here of course.
return;
}
}
}
}
static void
bse_pcm_device_jack_class_init (BsePcmDeviceJACKClass *klass)
{
GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
BseDeviceClass *device_class = BSE_DEVICE_CLASS (klass);
parent_class = g_type_class_peek_parent (klass);
gobject_class->finalize = bse_pcm_device_jack_finalize;
device_class->list_devices = bse_pcm_device_jack_list_devices;
const gchar *name = "jack";
const gchar *syntax = _("device");
"device" is too short of a syntax description for users.
i don't know jack devices, but i suppose you need to say something
like _("JACK_DEVICE_NAME") here. again, see how the alsa driver
tries to guide the user here.
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.
/* Being BSE_RATING_PREFFERED + 1, should do the right thing: if jackd is running,
* use it, if jackd isn't running (we'll get an error upon initialization,
* then), use the next best driver.
*
* Right now we will never start the jackd process ourselves.
*/
bse_device_class_setup (klass, BSE_RATING_PREFERRED + 1, name, syntax, info);
device_class->open = bse_pcm_device_jack_open;
device_class->close = bse_pcm_device_jack_close;
}
Cu... Stefan
for a few closing words:
first, thanks for starting out on the implementation of a jack
driver for BSE. the task in itself is definitely non-trivial.
on the first draft, it looks like the driver still needs a lot of
work to be basically usable.
some portions already look good, but in others...
it really was disappointing for me, how much of perfectly good
template code / logic provided by bsepcmdevice-alsa.c simply got
lost or ignored in the transition to a jack driver. re-doing these
just adds up to the list of TODOs.
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.
---
ciaoTJ
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
