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

[Olivier Guilyardi]

Attached: jack demuxer 0.13 "The Lucky One" (hopefully ;)

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

This is quite ugly IMO:
FIFO_PACKETS_NUM * sizeof(AVPacket) - av_fifo_size(my_fifo)

Plus one of my ringbuffers contains FIFO_PACKETS_NUM + 1 packets, making the
above statement even worse.

All ringbuffers implementations I know of have two functions, read_space() and
write_space(). The later, which misses in fifo.*, is very often needed and
allows to avoid ugly operations as above.

So my patch include a new function, av_fifo_space(), which completes
av_fifo_size() and sounds semantically great to me.

>> +    /* 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?

Right, that was a bug. Thanks!

> also (nitpick), id write
> latency / (self->nports * self->sample_rate)
> avoiding a slow division

Done

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

I now return the error reported by av_new_packet()

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

Not sure what you mean here.

> 
> [...]
>> +    self->timefilter  = ff_timefilter_new (1 / self->sample_rate, sqrt(2 * o), o * o);
> 
> is the division not missing a float or doubl cast?

Indeed ! Corrected..

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

Function removed and "pasted" into audio_read_packet()

> [...]
>> +    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 ?

Yes

> if not it is redundant

I reckon these three variables were too many. I've merged overrun and underrun
into pkt_xrun, and renamed xrun to jack_xrun.

There are many possible reasons for JACK xruns. They happen when jack fails to
read/write some data from/to the audio hardware. This may be caused by low
level/operating system overhead. But the most usual reasons are that the user
has not enabled realtime scheduling, or that he has set a jack buffer size which
is too low for his system to support it.

Whereas the pkt_xrun (formerly underrun || overrun) may happen when the non
realtime thread where audio_read_packet() gets called doesn't run fast enough to
follow the realtime thread. Apart from overhead situations, this could also
happen because the application blocks, etc...

So, IMO, it is important to make a difference between pkt_xrun and jack_xrun log
messages, to help debugging and ease user support. pkt_xrun may come from cpu
overhead or a bug in the application. jack_xrun is likely to be caused by a
wrong (or too cpu intensive) jack or kernel setup.

--
  Olivier



-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-r18171-jack-0.13.patch
Type: text/x-patch
Size: 15214 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090323/86c543c2/attachment.bin>