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

[Olivier Guilyardi]

Attached: timefilter libavformat patch 0.3

Among other things, the dependency on math.h has been dropped, and the stats are
gone.

Michael Niedermayer wrote:
> On Mon, Mar 02, 2009 at 09:22:31PM +0100, Olivier Guilyardi wrote:
>> Attached: timefilter libavformat patch version 0.2
> 
> [...]
>> +#include <stdlib.h>
>> +#include <math.h>
>> +#include <sys/time.h>
>> +#include <time.h>
>> +
>> +#include "timefilter.h"
>> +
>> +#define PI 3.14159265358979323846
> 
> other parts of lav* simply use M_PI without defining it ...

M_PI was undeclared even with <math.h>, apparently because of -std=c99.
Including "avformat.h" fixed that. Anyway, pi is not used anymore.

>> +
>> +struct TimeFilter {
>> +
>> +    /// Delay Locked Loop data. These variables refer to mathematical
>> +    /// concepts described in: http://www.kokkinizita.net/papers/usingdll.pdf
>> +    double tper;
>> +    double b;
>> +    double c;
>> +    double e2;
>> +    double t0;
>> +    double t1;
> 
> please use some english words for these variables

Corrected

> [...]
>> +TimeFilter * ff_timefilter_new(double period, double bandwidth)
>> +{
>> +    double o;
>> +    TimeFilter *self = calloc(1, sizeof(TimeFilter));
> 
> code in lav* uses av_malloz()

A leftover from libpendule. Fixed

>> +    self->tper = period;
>> +    o = 2 * PI * bandwidth * self->tper;
>> +    self->b = sqrt(2 * o);
>> +    self->c = o * o;
> 
>> +    self->t0 = 0;
> 
> useless

Removed

> [...]
>> +void ff_timefilter_update(TimeFilter *self, double system_time)
>> +{
>> +    double e;
>> +
>> +    if (!self->t0) {
>> +        // init loop
> 
>> +        self->e2 = self->tper;
> 
> this is incorrect in case of reset

You may be right yes, I moved this to ff_timefilter_new().

In the context of JACK, reset is meant to be called in case of XRUN. That means
we know nothing about the number of frames actually processed by the device
between the last and next cycles (the last and next calls to
ff_timefilter_update()), so that we can't update the loop, and need to reset it.
However, I indeed see no reason to reset the second integrator of the loop, and
thus lose its state which is the result of the training performed during the
cycles before the XRUN occured.

>> +        self->t0 = system_time;
>> +        self->t1 = self->t0 + self->e2;
>> +
>> +        // init stats
>> +        self->device_time = system_time;
>> +        self->system_period_error = self->filter_period_error = 0;
>> +        self->ncycles = 0;
> 
> id put all of above in reset and call reset from new

No, system_time is only available when calling ff_timefilter_update(), that is:
at the beginning of cycle. ff_timefilter_reset() may be called at a random
moment (in JACK this is when the xrun callback is called. Nothing guarantees
this happens at the beginning at a cycle).

The same thing happens with ff_timefilter_new(): the system time of the
beginning of the cycle is not available. This is especially true in the case of
JACK, because ff_timefilter_new() allocates some memory, and thus can't be
called from the realtime process callback. The moment the realtime process
callback gets called is the only event that relates to the beginning of a cycle.

> [...]
>> +/**
>> + * Various statistics as reported by ff_timefilter_stats()
>> + *
>> + * All values are in micro-seconds
>> + */
>> +typedef struct TimeFilterStats {
>> +    double filter_time;     /** last filtered time (same as ff_timefilter_read()) */
>> +    double filter_jitter;   /** last jitter between filtered and device clocks */
>> +    double filter_drift;    /** total drift between filtered and system clocks */
>> +    double next_filter_time;/** estimate of the next filtered time */
>> +
>> +    double system_time;     /** last system time */
>> +    double system_jitter;   /** last jitter between system and device clocks */
>> +
>> +    double device_time;     /** last device time */
>> +    double device_drift;    /** total drift between device and system clocks */
>> +} TimeFilterStats;
> 
> what use are these statistics except for debuging?
> And i surely agree they are very valuable for debug but they dont
> seem so usefull for normal use.

Stats are gone

>> +
>> +/**
>> + * Create a new DLL time filter
>> + *
>> + * Period must be the device cycle duration in micro-seconds. For example, at
> 
> considering that we are working with floats, the natural base unit is
> seconds not micro-
> besides the unit shouldnt matter for the filter
> 
> 
>> + * 44.1Hz and a buffer size of 512 frames, period = 512 / 44100. The filter
> 
>> + * only works if the cycle duration is fixed.
> 
> this should be trivial to fix

I don't think so. Anyway, this time filter is primarily meant for producing
timestamps when dealing with a hardware device clock, and resulting jitter and
drift. For instance, when dealing with soundcards you always read a fixed number
of samples at each cycle. I don't know whether this can be variable or not with
video capture devices.

>> + *
>> + * The bandwidth is up to you to choose. Smaller values will filter out more
>> + * of the jitter, but also take a longer time for the loop to settle. A good
>> + * starting point is something between 0.3 and 3 Hz.
> 
> the filter factors should be adjusted at the begin 

I don't understand this.

--
  Olivier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-r17780-timefilter-0.3.patch
Type: text/x-patch
Size: 7082 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090303/b8a16ae5/attachment.bin>