[FFmpeg-devel] [PATCH 1/4] avfilter/asrc_sine: increase phase precision

[Nicolas George]

Marton Balint (12024-11-06):
> Feel free to test this yourself, but I agree with others in this thread,
> testing on or optimizing for native 32bit CPUs is not something you can
> reasonably expect from fellow developers to spend time on. Also I would like
> to point out that accuracy and speed is naturally a tradeoff, and anybody
> interested could easily optimize the filter further at least for integer
> frequencies by pre-calculating sample_rate number of samples...

It is saddening to observe the pride in good efficient code in this
project decline along with the move towards more mercenary development
practices.

> I think the biggest problem with the current implementation is that the
> output is not periodic because of the impreciseness of the delta phase. A
> user can reasonably expect that the output of the sine source is bitexact
> periodic (as long as the frequency is a sane integer), but right now after a
> few seconds bitexact periodicity is lost, which is highly counterintuitive.
> 
> I would like to have the sine source fixed before pushing the aloop fate
> tests, because that depends on it. I will reply to this mail with an
> alternate patch which fixes the periodicity issue by correcting the phase
> every second. I can use that instead of the original patch if you prefer.

Thanks for explaining.

You have gotten the math wrong, and therefore are going for an incorrect
fix. Sure, the 32 bits of precision will push the incorrectness very far
in the future, but it is still invalid.

The output is not supposed to be periodical for any integer frequency
(assuming you mean periodical over one period; everything finite is
eventually periodical). For example, “sine=1000” will NOT be periodical
over a period, only after the tenth period.

The frequencies that should cause the output to be periodical are the
frequencies that are integer divisors of the sample rate.

Sure, even for a frequency of 900, the current code uses dphi=87652394
instead of dphi=4294967296/49=~87652393.80.

Once stated that way, the correct way of getting a periodical output is
obvious: you need to implement rational arithmetic.

In this particular case, it is quite easy:

1. Remove the +0.5 in the computation of dphi to get it to round
   downwards, leave everything else as is.

2. Convert ldexp(sine->frequency, 32) / sine->sample_rate to a rational
   to get a denominator, and subtract the integer part to keep only the
   fractional part, let us call it dphi_rem / dphi_den, with 0 <=
   dphi_rem < dphi_den.

4. Start with phi_rem = -phi_den / 2 (that is the +0.5 we removed).

5. Each time you add dphi to phi:

   5.1. Add dphi_rem to phi_rem.

   5.2. If phi_rem >= 0, subtract dphi_den to it and increment phi.

Regards,

-- 
  Nicolas George