[FFmpeg-devel] [PATCH] libavdevice: JACK demuxer

[Michael Niedermayer]

On Thu, Mar 19, 2009 at 01:30:48AM +0100, Olivier Guilyardi wrote:
> Attached: jack demuxer patch 0.12
> 
> Diego Biurrun wrote:
> > On Tue, Mar 17, 2009 at 02:11:07PM +0100, Olivier Guilyardi wrote:
> >> Diego Biurrun wrote:
> >>> On Tue, Mar 17, 2009 at 12:40:13PM +0100, Olivier Guilyardi wrote:
> >>>> Diego Biurrun wrote:
[...]
> +/**
> + * Size of the internal FIFO buffers as a number of audio packets
> + */
> +#define FIFO_PACKETS_NUM 16
> +
> +typedef struct {
> +    jack_client_t *     client;
> +    int                 activated;
> +    sem_t               packet_count;
> +    jack_nframes_t      sample_rate;
> +    jack_nframes_t      buffer_size;
> +    jack_port_t **      ports;
> +    int                 nports;
> +    TimeFilter *        timefilter;
> +    AVFifoBuffer *      new_pkts;

> +    unsigned int        new_pkts_size;
> +    AVFifoBuffer *      filled_pkts;

> +    unsigned int        filled_pkts_size;

maybe ive been too tired (yes i suggested it) but these make no sense
FIFO_PACKETS_NUM should be used


> +    int                 overrun;
> +    int                 underrun;
> +    int                 xrun;
> +} JackData;
> +
> +static int process_callback(jack_nframes_t nframes, void *arg)
> +{
> +    /* Warning: this function runs in realtime. One mustn't allocate memory here
> +     * or do any other thing that could block. */
> +
> +    int i, j;
> +    JackData *self = arg;
> +    float * buffer;
> +    jack_nframes_t latency, cycle_delay;
> +    AVPacket pkt;
> +    float *pkt_data;
> +    double cycle_time;
> +
> +    if (!self->client)
> +        return 0;
> +
> +    /* The approximate delay since the hardware interrupt as a number of frames */
> +    cycle_delay = jack_frames_since_cycle_start(self->client);
> +
> +    /* Retrieve filtered cycle time */
> +    cycle_time = ff_timefilter_update(self->timefilter,
> +                                      (double) av_gettime() / 1000000.0 - cycle_delay / self->sample_rate,
> +                                      self->buffer_size);
> +
> +    /* Check if an empty packet is available, so that we can fill it with audio data */
> +    if (av_fifo_size(self->new_pkts) < sizeof(pkt)) {
> +        self->underrun = 1;
> +        return 0;
> +    }
> +
> +    /* Check if there's enough space to send the filled packet */
> +    if (self->filled_pkts_size - av_fifo_size(self->filled_pkts) < sizeof(pkt)) {
> +        self->overrun = 1;
> +        return 0;
> +    }
> +
> +    /* Retrieve empty (but allocated) packet */
> +    av_fifo_generic_read(self->new_pkts, &pkt, sizeof(pkt), NULL);
> +
> +    pkt_data  = (float *) pkt.data;
> +    latency   = 0;
> +
> +    /* Copy and interleave audio data from the JACK buffer into the packet */
> +    for (i = 0; i < self->nports; i++) {
> +        latency += jack_port_get_total_latency(self->client, self->ports[i]);
> +        buffer = jack_port_get_buffer(self->ports[i], self->buffer_size);
> +        for (j = 0; j < self->buffer_size; j++) {
> +            pkt_data[j * self->nports + i] = buffer[j];
> +        }
> +    }
> +

> +    /* Timestamp the packet with the cycle start time minus the average latency */
> +    pkt.pts = (cycle_time - (latency / self->nports) / self->sample_rate) * 1000000.0;

isnt
(latency / self->nports) / self->sample_rate
an integer in seconds aka pretty inaccurate?
also (nitpick), id write
latency / (self->nports * self->sample_rate)
avoiding a slow division

[...]
> +static int supply_new_packets(JackData *self, AVFormatContext *context)
> +{
> +    AVPacket pkt;
> +    int pkt_size = self->buffer_size * self->nports * sizeof(float);
> +
> +    /* Supply the process callback with new empty packets, by filling the new
> +     * packets FIFO buffer with as many packets as possible. process_callback()
> +     * can't do this by itself, because it can't allocate memory in realtime. */
> +    while (self->new_pkts_size - av_fifo_size(self->new_pkts) >= sizeof(pkt)) {
> +        if (av_new_packet(&pkt, pkt_size) < 0) {
> +            av_log(context, AV_LOG_ERROR, "Could not create packet of size %d\n", pkt_size);
> +            return AVERROR(EIO);

the error should be either the one returned by av_new_packet() or ENOMEM
which is what the error would be


> +        }
> +        av_fifo_generic_write(self->new_pkts, &pkt, sizeof(pkt), NULL);
> +    }
> +    return 0;
> +}
> +
> +static int start_jack(AVFormatContext *context, AVFormatParameters *params)
> +{
> +    JackData *self = context->priv_data;
> +    jack_status_t status;
> +    int i, test;
> +    double o, period;
> +
> +    /* Register as a JACK client, using the context filename as client name. */
> +    self->client = jack_client_open(context->filename, 0, &status);
> +    if (!self->client) {
> +        av_log(context, AV_LOG_ERROR, "Unable to register as a JACK client\n");
> +        return AVERROR(EIO);
> +    }
> +
> +    sem_init(&self->packet_count, 0, 0);
> +

> +    self->overrun     = 0;
> +    self->xrun        = 0;
> +    self->activated   = 0;

redundant

[...]
> +    self->timefilter  = ff_timefilter_new (1 / self->sample_rate, sqrt(2 * o), o * o);

is the division not missing a float or doubl cast?


[...]
> +static int activate_jack(JackData *self, AVFormatContext *context)
> +{
> +    if (!jack_activate(self->client)) {
> +        self->activated = 1;
> +    } else {
> +        av_log(context, AV_LOG_ERROR, "Unable to activate JACK client\n");
> +        return -1;
> +    }
> +
> +    av_log(context, AV_LOG_INFO, "JACK client registered and activated (rate=%dHz, buffer_size=%d frames)\n",
> +           self->sample_rate, self->buffer_size);
> +
> +    return 0;

that stuff would fit nicer in the if() so both sides end in return

anyway, id this function really needed?
simply calling jack_activate() and setting activated seems simpler


[...]
> +    if (self->overrun) {
> +        av_log(context, AV_LOG_WARNING, "Audio packet overrun\n");
> +        self->overrun = 0;
> +    }
> +
> +    if (self->underrun) {
> +        av_log(context, AV_LOG_WARNING, "Audio packet underrun\n");
> +        self->underrun = 0;
> +    }
> +
> +    if (self->xrun) {
> +        av_log(context, AV_LOG_WARNING, "JACK xrun\n");
> +        self->xrun = 0;
> +    }

can xrun be != overrun || underrun ?
if not it is redundant

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090319/c8168a9e/attachment.pgp>