[pulseaudio-discuss] [RFC] [PATCH] tests: Add a latency measurement test

[Arun Raghavan]

On Tue, 2013-05-21 at 17:13 +0200, Peter Meerwald wrote:
> Hello, 
> 
> > This test is intended to measure real latency by playing a sample to a
> > sink and capturing that over a loopback interface. The loopback can
> > either be physical (cable running from headphone out to line in) or
> > virtual (monitor source or module loopback).
> 
> nice idea :)
> 
> the sine tone in the beginning has chirps, intended?

Thanks for the review. :)

I did notice some discontinuities in Audacity. Need to fix that.

> the program's output could be clearer; it is not obvious what is going on 
> ('giving up' but the program does not stop?):
> 
> Capture signal too weak at 100% volume (0.766978). Giving up.
> Capture signal too weak at 100% volume (0.748493). Giving up.
> Capture signal too weak at 100% volume (0.797834). Giving up.
> Underflow
> Capture signal too weak at 100% volume (0.418111). Giving up.
> Too much noise on capture (0.606354). Giving up.
> Latency 10565
> Latency 10678
> Latency 10709

Haha, it should've asserted out, but I guess the assert isn't working on
your machine?

Also, is the test actually working despite it not being able to clearly
figure out when there's playback and when there's silence? I've picked
what parameters seemed sensible and worked here, but there might be room
to tweak them (the actual RMS values there make it look like the test
should not work, though).

> +#define TONE_HZ SAMPLE_HZ / 100
> 
> maybe #define TONE_HZ (SAMPLE_HZ / 100)

Ack on this and subsequent.

> +#define PLAYBACK_LATENCY 25 /* ms */
> +#define CAPTURE_LATENCY 5 /* ms */
> 
> > +static const char *bname = NULL;
> 
> bname is not very clear; what is it for?

The variable name's a hold over from one of the other tests. It sets the
name for the pa_context.

> > +static float out[N_OUT][CHANNELS];
> > +static int ppos = 0;
> > +
> > +static int n_underflow = 0;
> > +static int n_overflow = 0;
> 
> unsigned maybe?
> 
> > +static inline float rms(const float *s, int n) {
> > +    float sq = 0;
> > +    int i;
> 
> float sq = 0.0f;
> unsigned i, unsigned n?

I usually use int for brevity if overflows are not going to crop up.

> > +
> > +    for (i = 0; i < n; i++)
> > +        sq += s[i] * s[i];
> > +
> > +    return sqrt(sq / n);
> 
> should be sqrtf() since sq is float

Ack.

> > +#define WINDOW 2 * CHANNELS
> 
> #define WINDOW (2 * CHANNELS)
> 
> > +static void read_cb(pa_stream *s, size_t nbytes, void *userdata) {
> > +    static float last = 0.0;
> 
> static float last = 0.0f;
> 
> > +    const float *in;
> > +    float cur;
> > +    int r;
> > +    unsigned int i = 0;
> 
> here we have an unsigned?

That's for the comparison with size_t (which otherwise causes a
signedness warning).

> > +        /* We leave the definition of 0 generous since the window might
> > +         * straddle the 0->1 transition, raising the average power. We keep the
> > +         * definition of 1 tight in this case and detect the transition in the
> > +         * next round. */
> > +        if (last < 0.5 && cur > 0.8) {
> 
> I'd append an f to denote float precision, so 0.5f and 0.8f

Sure.

> > +            pa_gettimeofday(&tv_in);
> > +            fprintf(stderr, "Latency %llu\n", (unsigned long long) pa_timeval_diff(&tv_in, &tv_out));
> 
> is the cast necessary?

I've see a compiler warning without that.

> > +enum {
> > +    CALIBRATION_ONE,
> > +    CALIBRATION_ZERO,
> > +    CALIBRATION_DONE,
> > +};
> > +
> > +static int cal_state = CALIBRATION_ONE;
> > +
> > +static void calibrate_write_cb(pa_stream *s, size_t nbytes, void *userdata) {
> > +    int i, r, nsamp = nbytes / fs;
> > +    float tmp[nsamp][2];
> > +    static int count = 0;
> > +
> > +    /* Write out a sine tone */
> > +    for (i = 0; i < nsamp; i++)
> > +        tmp[i][0] = tmp[i][1] = cal_state == CALIBRATION_ONE ? sin(count++ * TONE_HZ * 2 * M_PI / SAMPLE_HZ) : 0.0;
> 
> sinf() since float, 0.0f
> 
> 
> > +START_TEST (loopback_test) {
> > +    pa_mainloop* m = NULL;
> > +    int i, ret = 0, pulse_hz = N_OUT / 1000;
> 
> division rounds towards zero, intended?

Shouldn't matter. Was just aiming for a 10ms bip. Will rewrite as
SAMPLE_HZ / 1000.

> > +
> > +    /* Generate a square pulse */
> > +    for (i = 0; i < N_OUT; i++)
> > +        if (i < pulse_hz)
> > +            out[i][0] = out[i][1] = 1.0;
> > +        else
> > +            out[i][0] = out[i][1] = 0.0;
> 

-- Arun