[FFmpeg-devel] [PATCH] libavfilter: add atempo filter (revised patch v5)

[Pavel Koshevoy]

On 06/12/2012 05:07 PM, Michael Niedermayer wrote:
> On Mon, Jun 11, 2012 at 09:18:02PM -0600, Pavel Koshevoy wrote:

[...]

>> +/**
>> + * Prepare filter for processing audio data of given format,
>> + * sample rate and number of channels.
>> + */
>> +static int yae_reset(ATempoContext *atempo,
>> +                     enum AVSampleFormat format,
>> +                     int sample_rate,
>> +                     int channels)
>> +{
>> +    const int sample_size = av_get_bytes_per_sample(format);
>> +    uint32_t nlevels  = 0;
>> +    uint32_t pot;
>> +    int i;
>> +
>> +    atempo->format   = format;
>> +    atempo->channels = channels;
>> +    atempo->stride   = sample_size * channels;
>> +
>> +    // pick a segment window size:
>> +    atempo->window = sample_rate / 24;
>> +
>> +    // adjust window size to be a power-of-two integer:
>> +    nlevels = av_log2(atempo->window);
>> +    pot = 1<<  nlevels;
>> +    av_assert0(pot<= atempo->window);
>> +
>> +    if (pot<  atempo->window) {
>> +        atempo->window = pot * 2;
>> +        nlevels++;
>> +    }
>> +
>> +    // initialize audio fragment buffers:
>> +    REALLOC_OR_FAIL(atempo->frag[0].data,
>> +                    atempo->window * atempo->stride);
>> +
>> +    REALLOC_OR_FAIL(atempo->frag[1].data,
>> +                    atempo->window * atempo->stride);
>> +
>> +    REALLOC_OR_FAIL(atempo->frag[0].xdat,
>> +                    atempo->window * 2 * sizeof(FFTComplex));
>> +
>> +    REALLOC_OR_FAIL(atempo->frag[1].xdat,
>> +                    atempo->window * 2 * sizeof(FFTComplex));
>> +
>> +    // initialize FFT contexts:
>> +    av_fft_end(atempo->fft_forward);
>> +    av_fft_end(atempo->fft_inverse);
> maybe this should call uninit()
>
> also if something in this fails then the resuslting state is a mess
> some arrays one size some others another some memleaks and the ffts
> might even end with a double free i think

I'll look into that, thank you.

>
>
>> +
>> +    atempo->fft_forward = av_fft_init(nlevels + 1, 0);
>> +    if (!atempo->fft_forward) {
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    atempo->fft_inverse = av_fft_init(nlevels + 1, 1);
>> +    if (!atempo->fft_inverse) {
>> +        return AVERROR(ENOMEM);
>> +    }
> by using a RDFT you can cut the computations needed down by a factor
> of 2

My DSP experience is limited.  What is the procedure for computing cross 
correlation of two signals using rDFT?

[...]

>> +            // build a multi-resolution pyramid for fragment alignment:
>> +            yae_downmix(atempo, yae_curr_frag(atempo));
> i see no multi-resolution pyramid

Sorry about that, it's a stale comment.  I wasn't happy with the results of 
multi-resolution pyramid approach as I originally intended it.  At coarse 
resolution levels there often wasn't enough information for a good initial 
alignment, and the refinement at higher resolution levels couldn't always 
compensate.  I also implemented coarse-to-fine approach for calculating cross 
correlation peaks using the multi-resolution pyramid.  It works pretty well, but 
since it isn't SIMD optimized it's still slower than using av_fft.

>
> overall i have the feeling this filter could be implemented in less
> code and with less complexity but thats not an objection from me.

In my opinion this is already simpler than soundtouch or mplayer scaletempo 
filter, and it sounds better than both.


>
> ill leave further review&  applying to stefano
>
> if you want to maintain this filter once its
> in git, then you should get a public git clone of ffmpeg (for example
> on github) so you can conveniently maintain it. And once you have
> some changes in your git repository with which you are happy just
> ask me to merge them ...
>

I am not sure if it's really worth it for just one file.  Why shouldn't I keep a 
private branch as I do now and simply send a patch when I am ready?

Thank you,
     Pavel.