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

Sat Feb 28 19:38:49 CET 2009

On Sat, Feb 28, 2009 at 02:41:51PM +0100, Olivier Guilyardi wrote:
> Michael Niedermayer a ?crit :
> > On Sat, Feb 28, 2009 at 01:46:21AM +0100, Olivier Guilyardi wrote:
> >> Version 0.6 of the jack demuxer path is attached.
> > [...]
> >>>> +            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.
> > 
> > we need timestamps that are compareable to the "system clock/wall clock" so
> > audio/video sync is possible. We also need accurate timestamps and
> > av_gettime() is probably not accurate enough for many uses.
> > I assume the running counters count samples and not true time?
> > If so JACK has the same problem OSS has.
> 
> JACK does feature some sophisticated time computing routines, but nothing for 
> absolute (system/wall) timestamps.
> 
> > this needs something like
> > 
> > estimated_time = av_gettime() - (latency + cycle_delay) * self->frame_ms; (taken from your code)
> > 
> > pts= last_pts + samples_since_last_time / sample_rate
> > pts+= (estimated_time - pts)*0.01;
> > sample_rate+= (samples_since_last_time/estimated_time_since_last - sample_rate)*0.001;
> 
> I have something better to propose you: Delay Locked Loop (DLL) time filtering. 
> I didn't use it at first, because I was unsure that it fitted the job, and to 
> avoid creating some confusion.
> 
> More about it by Fons Adriaensen:
> http://www.kokkinizita.net/papers/usingdll.pdf

this pretty much describes what i suggested (from looking at the equations, i
didnt read through the text) and i would have called it alpha/beta filter but
maybe there are some differences ive not checked the equations for being
strictly equivalent.

> 
> > also as our OSS code doesnt have something similar yet, you would possibly
> > make some people happy if you could implement this in a seperate file, & patch
> > and it could be used by both ...
> > (note the 0.001 / 0.01 constants are arbitary picked by me and likely poor
> >  choices, and above code is untested ...)
> 
> You are in a lucky day, I implemented DLL filtering as a small standalone 
> library, libpendule, a few weeks ago:
> 
> http://svn.samalyse.com/pendule/trunk/
> 
> We've done quite a lot of measuring on various hardware (see the README about 
> ./measure and ./graph), and the results are very good in regard to jitter reduction.
> 
> libpendule actually consists into two very small files, pendule.h and pendule.c, 
> with no dependency. It could be used in the oss demuxer, and possibly others. 
> Plus, I'll make you a good price ;) Do you buy?

1. i dont know what "CeCILL-B Free Software License Agreement version 1.0"
is or if this is GPL&LGPL compatible or not

2. it seems limited to constant sized packets, my example above is not

3. i dont like the code, its too large for what it does, the variables
   are poorly named, half of them being 1 or 2 letters,
   spagethi code like:
    double device_rate_error;
    stats->filter_time = self->filter_time;
    stats->next_filter_time = self->t1;
    stats->system_time = self->system_time;
    stats->device_time = self->device_time;
    stats->filter_drift = self->filter_time - self->system_time;
    stats->device_drift = stats->device_time - self->system_time;
    device_rate_error = self->ncycles ? stats->device_drift / self->ncycles : 0;
    stats->filter_jitter = self->filter_period_error - device_rate_error;
    stats->system_jitter = self->system_period_error - device_rate_error;

    can anyone read this without reformating it first?

the 2 parameters that control the filter are locked together using a
formular using sqrt() PI and others, while iam sure this can be proofen
to be optimal in a idealized case its certainly not in all practical
cases, the user should have the chance to set both independantly, and
as a sideeffect the dependancy on math.h can then be droped
...

anyway if you want to submit that code as patch for libavformat
under LGPL and dont mind my mercyless critique then it is very
welcome but i would surely insist on some cleanup

> 
> > 
> > [...]
> >>>> +    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, 
> > 
> > is the ringbuffer stuff doing a copy on its own? if so maybe you should
> > not use it but rather directly copy & interleave the data into AVPackets.
> 
> Yes, the way I use the ringbuffer implies two copy operations (w+r).
> 
> There is an important point to consider: the process_callback function is 
> running in a thread with realtime priority, and mustn't do anything that could 
> block. In particular, allocating memory (and thus packets) in there isn't an 
> option at all.

no need for that at all

read_packet(){
    while(no packet available) ;
    n= (n+1) % ring_size
    packet= ring[n];
    ring[n]= allocate new packet;

    return packet;
}

realtime_proc(){
    if(n!=m){
        packet= ring[m];
        copy&interleave data from hw into packet
        m= (m+1) % ring_size;
    }
}

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20090228/229c0aba/attachment.pgp>