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

[Olivier Guilyardi]

Attached:
- jack demuxer patch 0.14
- av_fifo_space patch

Michael Niedermayer wrote:
> On Mon, Mar 23, 2009 at 08:17:57PM +0100, Olivier Guilyardi wrote:
>>
>> 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.
> 
> should be a seperate patch

Attached

> [...]
>>>> +    self->overrun     = 0;
>>>> +    self->xrun        = 0;
>>>> +    self->activated   = 0;
>>> redundant
>> Not sure what you mean here.
> 
> they should be 0 alraedy

Okay, removed.

> [...]
> 
>> +    /* Retrieve filtered cycle time */
>> +    cycle_time = ff_timefilter_update(self->timefilter,
>> +                                      (double) av_gettime() / 1000000.0 - cycle_delay / self->sample_rate,
>> +                                      self->buffer_size);
> 
> cycle_delay / self->sample_rate is a integer i belive
> and the (double) cast is useless where it is

Right, fixed. Sorry

> [...]
>> +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->pkt_xrun    = 0;
>> +    self->jack_xrun   = 0;
>> +    self->activated   = 0;
>> +    self->sample_rate = jack_get_sample_rate(self->client);
> 
>> +    self->nports      = params->channels;
> 
> isnt channels a better name?

Not really. JACK deals with ports, not channels. A JACK client can (and often)
have many input/output ports. Each port consumes or produces a mono audio
stream. For example a multi-track application usually registers one or two ports
for each of its track, etc... The user can then connect ports in the way he/she
wishes.

There are some conventions for mapping ports to so-called channels, such as
appending _L, _R, or _1 and _2 to port names. But AFAIK these conventions are
not reflected in any way in the implementation and api of JACK.

In the context of ffmpeg, there indeed are channels, not ports. But to me, the
job of this demuxer is to bridge ffmpeg and jack, that is:
- deal with ffmpeg on one side
- deal with jack on the other side
- connect both sides, and be explicit about the peculiarities of this connection

This last point is well achieved by the following statement IMO :

self->nports = params->channels;

--
  Olivier

-------------- next part --------------
A non-text attachment was scrubbed...
Name: av_fifo_space-r18313.patch
Type: text/x-patch
Size: 1041 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090402/548b41a7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-r18313-jack-0.14.patch
Type: text/x-patch
Size: 14048 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090402/548b41a7/attachment-0001.bin>