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

[Olivier Guilyardi]

Version 0.6 of the jack demuxer path is attached.

Michael Niedermayer wrote:

> trailing whitespace
> ffmpeg-r17647-jack-0.5.patch:18:+check_header jack/jack.h && 

This line is gone, since check_header was redundant with check_lib2.

> 
> x==0 / x!=0 can be simplified to !x / x
> ffmpeg-r17647-jack-0.5.patch:168:+    if (jack_activate(self->client) != 0) {
> ffmpeg-r17647-jack-0.5.patch:203:+    if ((test = start_jack(context, params)) != 0)

Fixed

> static prototype, maybe you should reorder your functions
> ffmpeg-r17647-jack-0.5.patch:106:+static void create_ringbuffers(PrivateData *self);
> ffmpeg-r17647-jack-0.5.patch:107:+static int  start_jack(AVFormatContext *context, AVFormatParameters *params);
> ffmpeg-r17647-jack-0.5.patch:108:+static void stop_jack(PrivateData *self);
> ffmpeg-r17647-jack-0.5.patch:110:+static int  audio_read_header(AVFormatContext *context, AVFormatParameters *params);
> ffmpeg-r17647-jack-0.5.patch:111:+static int  audio_read_packet(AVFormatContext *context, AVPacket *pkt);
> ffmpeg-r17647-jack-0.5.patch:112:+static int  audio_read_close(AVFormatContext *context);
> ffmpeg-r17647-jack-0.5.patch:114:+static int  process_callback(jack_nframes_t nframes, void *arg);
> ffmpeg-r17647-jack-0.5.patch:115:+static void shutdown_callback(void *arg);
> ffmpeg-r17647-jack-0.5.patch:116:+static int  xrun_callback(void *arg);

They're all gone.

> Non static with no ff_/av_ prefix
> ffmpeg-r17647-jack-0.5.patch:343:+AVInputFormat jack_demuxer = {
> 
> missing } prior to else
> ffmpeg-r17647-jack-0.5.patch:243:+        else if (info.size < nframes)

Fixed

> 
>  Non doxy comments
> ffmpeg-r17647-jack-0.5.patch:63:+#define _BSD_SOURCE 1 // for usleep() from unistd.h
> --
> ffmpeg-r17647-jack-0.5.patch-77-+
> ffmpeg-r17647-jack-0.5.patch-78-+// Size of the internal ringbuffer as a number of jack buffers
> ffmpeg-r17647-jack-0.5.patch:79:+#define RING_NBUFFERS 4
> ffmpeg-r17647-jack-0.5.patch-80-+

Fixed

>> +#define _BSD_SOURCE 1 // for usleep() from unistd.h
>> +#include "config.h"
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include <unistd.h>
>> +#include <jack/jack.h>
>> +#include <jack/ringbuffer.h>
> 
> that are many headers, are all needed?

Most of them are gone.

>> +typedef struct {
>> +    jack_client_t * volatile  client;
>> +    jack_nframes_t            sample_rate;
> 
>> +    double                    frame_ms;
> 
> time related things should be calculated exactly, doubles are not,
> besides they result in differences between platforms

This should hopefully be fixed.

>> +    int rb_size;
>> +
>> +    rb_size = RING_NBUFFERS * sizeof(PacketControlInfo);
> 
> declaration & init can be merged

Fixed

>> +    self->nports = params->channels;
>> +    self->ports = av_malloc(self->nports * sizeof(*self->ports));
>> +    self->buffer_size = jack_get_buffer_size(self->client);
>> +    self->period = self->buffer_size * self->frame_ms;
> 
> these assignments can be vertically aligned prettier

Aligned

>> +static void stop_jack(PrivateData *self)
>> +{
>> +    if (self->client) {
>> +        jack_deactivate(self->client);
>> +        jack_client_close(self->client);
>> +    }
>> +    jack_ringbuffer_free(self->ctl_rb);
>> +    jack_ringbuffer_free(self->data_rb);
> 
>> +    av_free(self->ports);
> 
> i generally prefer av_freep() as its safer against mistakes

Fixed

>> +            latency = latency / self->nports;
>> +            cycle_delay = jack_frames_since_cycle_start(self->client);
>> +            info.pts = av_gettime() - (latency + cycle_delay) * self->frame_ms;
> 
> does jack not provide a timestamp for the returned audio?

Jack doesn't provide any absolute timestamp. Only some running counters. This is
one of the reasons why I asked whether timestamps should be absolute or relative
in an earlier post.

>> +    while ((timeout > 0) && self->client
>> +           && (jack_ringbuffer_read_space(self->ctl_rb) < sizeof(info))) {
>> +        usleep(self->period);
>> +        timeout -= self->period;
>> +    }
> 
> as has been said the use of usleep() is not pretty
> also no blocking should happen when AVFMT_FLAG_NONBLOCK is set

The usleep() based polling is gone. I changed this to a semaphore, as advised on
jack-devel, where I was told that sem_post() is realtime safe.

>> +    for (i = 0; i < self->nports; i++) {
>> +        jack_ringbuffer_read(self->data_rb, (char *) self->channel_buffer,
>> +                             info.size * sizeof(float));
>> +        for (j = 0; j < info.size; j++) {
>> +            output_data[j * self->nports + i] = self->channel_buffer[j];
>> +        }
>> +    }
> 
> useless copy?
> why not allocate a packet (or several) and read directly into them?

The data needs to be interleaved. Doing this efficiently in-place is non
trivial, because there might be more than 2 input channels. In other terms this
is a matrix transposition with an unknown number of rows and columns. For
example, the GNU Scientific Library offers a function for this:
gsl_matrix_transpose()

Is it an option?

--
  Olivier

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-r17653-jack-0.6.patch
Type: text/x-patch
Size: 11342 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090228/b025c389/attachment.bin>