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

[Pavel Koshevoy]

On 06/07/2012 03:00 AM, Stefano Sabatini wrote:
> Nit: commit title may be:
> libavfilter: add atempo filter

done

[...]

>> +/**
>> + * @file
>> + * tempo scaling audio filter -- an implementation of WSOLA algorithm
>> + */
> please mention the original source, if the filter was based on another
> implementation

Done.  The original source is my video player, Apprentice Video, hosted on 
sourceforge.



[...]
>> +typedef enum {
>> +    kLoadFragment     = 0,
>> +    kAdjustPosition   = 1,
>> +    kReloadFragment   = 2,
>> +    kOutputOverlapAdd = 3,
>> +    kFlushOutput      = 4
> what's the "k" good for?

In QuickTime and other frameworks I've worked with k prefix denotes a constant 
value (a const variable or enumeration value).  However, since the mere presence 
of this prefix raised a question I have removed it.


>> +
>> +} TState;
> the "T" in TAudioFragment and TState is weird.

I used to put  _t on all my type names, however I later discovered that _t is 
reserved for POSIX types.  When I switched to CamelCase a lot of the code I 
encountered was prefixed with C (for class, I guess).  I prefer to prefix type 
names with T instead, because it applies equally well to a struct, class, enum, 
typedef...  Anyway, since it looks odd to you I have renamed TAudioFragment to 
AudioFragment, and TState to FilterState.

[...]

>> +/**
>> + * Initialize filter state.
>> + */
>> +static void yae_constructor(ATempoContext *atempo)
> what "yae" stands for?

yae is the namespace name I use in the original C++ version of this filter.  Yae 
is a letter in the Cyrillic script -- 
http://en.wikipedia.org/wiki/Yae_%28Cyrillic%29


> Nit: the name "yae_constructor" sounds weird, the predominant
> convention is to use a verbal form rather than a noun for naming a
> function (like: yae_build or yae_init).

Done.  I got rid of yae_constructor and moved relevant code to init(..) 
instead.  I also got rid of yae_destructor and moved relevant code to uninit(..)


>> +{
>> +    atempo->ring = 0;
>> +    atempo->size = 0;
>> +    atempo->head = 0;
>> +    atempo->tail = 0;
>> +
>> +    atempo->format = AV_SAMPLE_FMT_NONE;
>> +    atempo->channels = 0;
>> +
>> +    atempo->window = 0;
>> +    atempo->tempo = 1.0;
>> +    atempo->drift = 0;
>> +
>> +    memset(&atempo->frag[0], 0, sizeof(atempo->frag));
>> +
>> +    atempo->nfrag = 0;
>> +    atempo->state = kLoadFragment;
>> +
>> +    atempo->position[0] = 0;
>> +    atempo->position[1] = 0;
>> +
>> +    atempo->fft_forward = NULL;
>> +    atempo->fft_inverse = NULL;
>> +    atempo->correlation = NULL;
>> +
>> +    atempo->request_fulfilled = 0;
>> +    atempo->dst_buffer = NULL;
>> +    atempo->dst = NULL;
>> +    atempo->dst_end = NULL;
>> +    atempo->nsamples_in = 0;
>> +    atempo->nsamples_out = 0;
> I think a memset to 0, followed by the explicit initialization of the
> few non-zero fields will be simpler.

I was assured that the filter context private data is memset to 0 before init is 
called, therefore I got rid of the redundant initialization.


>> +/**
>> + * Reset filter to initial state
>> + */
>> +static void yae_clear(ATempoContext *atempo)
>> +{
>> +    atempo->size = 0;
>> +    atempo->head = 0;
>> +    atempo->tail = 0;
>> +
>> +    atempo->drift = 0;
>> +    atempo->nfrag = 0;
>> +    atempo->state = kLoadFragment;
>> +
>> +    atempo->position[0] = 0;
>> +    atempo->position[1] = 0;
>> +
>> +    yae_clear_frag(&atempo->frag[0]);
>> +    yae_clear_frag(&atempo->frag[1]);
>> +
>> +    // shift left position of 1st fragment by half a window
>> +    // so that no re-normalization would be required for
>> +    // the left half of the 1st fragment:
>> +    atempo->frag[0].position[0] = -(int64_t)(atempo->window / 2);
>> +    atempo->frag[0].position[1] = -(int64_t)(atempo->window / 2);
>> +
>> +    if (atempo->dst_buffer) {
>> +        avfilter_unref_buffer(atempo->dst_buffer);
>> +        atempo->dst_buffer = NULL;
>> +        atempo->dst        = NULL;
>> +        atempo->dst_end    = NULL;
>> +    }
>> +
>> +    atempo->request_fulfilled = 0;
>> +    atempo->nsamples_in       = 0;
>> +    atempo->nsamples_out      = 0;
>> +}
> again this can be simplified / factorized with yae_constructor

Not really, this function can be called at any time after init(..), where as 
init(..) probably should be called only once.

[...]

>> +inline static TAudioFragment * yae_curr_frag(ATempoContext *atempo)
> nit: *yae_curr_frag, const ATempoContext *atempo

Done *yae_curr_frag(..), can't do const ATempoContext * because it will result 
in compiler warnings about discarded const qualifier.



>> +{
>> +    return&atempo->frag[atempo->nfrag % 2];
>> +}
>> +
>> +inline static TAudioFragment * yae_prev_frag(ATempoContext *atempo)
> ditto

same answer.



>> +{
>> +    return&atempo->frag[(atempo->nfrag + 1) % 2];
>> +}
>> +
>> +/**
>> + * Find the minimum of two scalars
>> + */
>> +#define yae_min(TScalar, a, b)                          \
> Uh? where was TScalar defined?
> Also can't you use FFMAX/FFMIN for such purposes?

   yae_min(TScalar, a, b)

is analogous to C++

   template <typename TScalar>
   const TScalar &
   std::min(const TScalar & a, const TScalar & b)

Here, TScalar is a template parameter specifying the type of function parameters.

Anyway, I was able to use FFMIN and FFMAX instead.

[...]

>> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
>> +{
>> +    ATempoContext *atempo = ctx->priv;
>> +    yae_constructor(atempo);
>> +
>> +    if (args) {
>> +        char   *tail = NULL;
>> +        double tempo = av_strtod(args,&tail);
> please check that tail is NULL&  fail otherwise like "Invalid scale
> factor '%s' provided, good for catching typos.

done


>> +/**
>> + * Deallocate any memory held by the filter,
>> + * release any buffer references, etc.
>> + *
>> + * This does not need to deallocate the AVFilterContext::priv memory itself.
>> + */
> Nit: generic comments are good in template code (and maybe we need
> it), but they are usually a bit annoying to deal with in working code
> (e.g. when you update API).

Agreed, although I've kept some of the comments as a reminder of what the less 
obvious functions are supposed to do.

[...]

>
>> +    enum AVSampleFormat format = (enum AVSampleFormat)inlink->format;
> unnecessary cast?

fixed


[...]

>> +/**
>> + * Samples filtering callback that.
>> + * This is where a filter receives audio data and processes it.
>> + */
> same comment as above

generic comment removed


> [...]
>> diff --git a/libavfilter/version.h b/libavfilter/version.h
>> index 76f649e..c90b4ad 100644
>> --- a/libavfilter/version.h
>> +++ b/libavfilter/version.h
>> @@ -30,7 +30,7 @@
>>
>>   #define LIBAVFILTER_VERSION_MAJOR  2
>>   #define LIBAVFILTER_VERSION_MINOR 78
>> -#define LIBAVFILTER_VERSION_MICRO 100
>> +#define LIBAVFILTER_VERSION_MICRO 101
> Minor bump, but leave this to the committer so you won't have to
> update again and again your patch.

I've removed my changes from libavfilter/version.h


> Thanks for the cool patch.


Thank you.  I have already submitted another patch incorporating the requested 
changes.

     Pavel.