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

[Michael Niedermayer]

On Mon, Mar 23, 2009 at 08:17:57PM +0100, Olivier Guilyardi wrote:
> 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.

should be a seperate patch


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

they should be 0 alraedy


[...]

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


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

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20090331/c6f45c99/attachment.pgp>