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

[Pavel Koshevoy]

On 06/10/2012 01:17 PM, Stefano Sabatini wrote:
> On date Friday 2012-06-08 09:16:36 -0600, Pavel Koshevoy encoded:
[...]

>> +/**
>> + * Filter state machine states
>> + */
>> +typedef enum {
>> +    YAE_LOAD_FRAGMENT      = 0,
>> +    YAE_ADJUST_POSITION    = 1,
>> +    YAE_RELOAD_FRAGMENT    = 2,
>> +    YAE_OUTPUT_OVERLAP_ADD = 3,
>> +    YAE_FLUSH_OUTPUT       = 4,
>> +
>> +} FilterState;
> Nit: no need to explicitely enumerate, C compiler will deal with it

Done.

[...]

>> +/**
>> + * Filter state machine
>> + */
>> +typedef struct {
>> +    // ring-buffer of input samples, necessary because some times
>> +    // input fragment position may be adjusted backwards:
>> +    uint8_t *buffer;
>> +
>> +    // ring-buffer maximum capacity,
>> +    // expressed as number of multi-channel sample units;
>> +    //
>> +    // for example, given stereo data 1 multi-channel sample unit
>> +    // refers to 2 samples for left/right channels:
>> +    int ring;
>> +
>> +    // ring-buffer house keeping:
>> +    int size;
>> +    int head;
>> +    int tail;
> Also note that we have several buffer implementations, in particular
> libavutil/fifo.h and audio_fifo.h, don't know if they could be used in
> this particular case.

I probably could have used the fifo.h because it provides av_fifo_peek2(..), 
although I would still need to keep some of the house keeping variables for 
input/output sample positions.  I don't want to switch now because the code is 
already written and tested.

[...]

>> +/**
>> + * This resets filter to initial state, does not deallocate existing buffers.
>> + */
> Nit++: Reset filter to initial state, do not deallocate ...

Done.

[...]

>> +/**
>> + * A helper macro for initializing complex data buffer with scalar data
>> + * of a given type.
>> + */
>> +#define yae_init_xdat(TScalar, scalar_max)                              \
> nit+: TScaler ->  scalar_type

Done.

[...]

>> +            FFTSample s;                                                \
>> +            FFTSample max;                                              \
>> +            FFTSample ti;                                               \
>> +            FFTSample si;                                               \
> nit++: this can be put on a single line, save some vertical space

Done.

[...]

>> +/**
>> + * Populate the internal data buffer on as-needed basis.
>> + *
>> + * @return 0 if requested data was already available or was loaded.
>> + * @return AVERROR(EAGAIN) if more input data is required.
> nit+++: avoid double @return, a single paragraph is fine (not that it
> gains much since this is internal)

Done.

[...]

>> +/**
>> + * Populate current audio fragment data buffer.
>> + *
>> + * @return 0 when the fragment is ready.
>> + * @return AVERROR(EAGAIN) if more input data is required.
> ditto

Done.

[...]


>> + */
>> +static int yae_load_frag(ATempoContext *atempo,
>> +                         const uint8_t **src_ref,
>> +                         const uint8_t *src_end)
>> +{
>> +    // shortcuts:
>> +    AudioFragment *frag = yae_curr_frag(atempo);
>> +    uint8_t *dst;
>> +    int64_t missing;
>> +    uint32_t nsamples;
>> +    int64_t start;
>> +    int64_t zeros;
>> +    int na;
>> +    int nb;
>> +    const uint8_t *a;
>> +    const uint8_t *b;
>> +    int i0;
>> +    int i1;
>> +    int n0;
>> +    int n1;
> nit++: try to group variables together, so we get a shorter
> declaration list (easier to read)

Done, although I disagree.  A list of variables with one variable per line can 
be easily scanned vertically and variables of different types can be grouped 
together.  When variables are grouped on one line based on their type 
readability suffers -- the list has to be scanned vertically and horizontally, 
and logical grouping of variables of different types may be disrupted.

[...]

>> +        if (tail&&  *tail) {
>> +            av_log(ctx, AV_LOG_ERROR, "invalid tempo value '%s'\n", args);
> Nit+++: "Invalid..."

Done.

[...]

>> +
>> +    ff_set_common_samplerates(ctx, formats);
> Nit++++: remove the previous empty line, so you group all the instructions

Done.

[...]

>> +/**
>> + * Check configuration properties of the input link
>> + * and update filters internal state as necessary.
>> + *
>> + * Return zero on success, AVERRROR(..) on failure.
>> + */
> feel free to drop this doxy

Done.

[...]

>> +            // adjust the PTS:
>> +            atempo->dst_buffer->pts =
>> +                av_rescale(outlink->time_base.den,
>> +                           atempo->nsamples_out,
>> +                           outlink->time_base.num * outlink->sample_rate);
> not sure but maybe av_rescale_q(atempo->nsamples_out, (AVRational){1, outlink->sample_rate}, outlink->time_base)
>
> may be safer, as it avoids a possible int*int ->  int64_t overflow:
> outlink->time_base.num * outlink->sample_rate

Done, although I line-wrapped it to be under 80-chars per line (helps readability).

I am more concerned about the fact that the PTS is computed from the number of 
samples output.  If this filter is used interactively with seeking and allowing 
user to change tempo continuously then this PTS calculation will not be enough.

[...]

>> +    avfilter_unref_buffer(src_buffer);
> avfilter_unref_bufferp should be safer

Done.

[...]

>> +}
>> +
>> +/**
>> + * Frame request callback.
>> + *
>> + * A call to this should result in at least one frame
>> + * being output over the given link.
>> + *
>> + * @return 0 on success.
>> + */

Removed.

[...]

>> +/**
>> + * Make the filter instance process a command.
>> + *
>> + * @param cmd   -- an alphanumeric command to process
>> + * @param arg   -- the argument for the command
>> + * @param res   -- a buffer with size res_size for filter response
>> + * @param flags -- command flags
>> + *
>> + * When AVFILTER_CMD_FLAG_FAST flag is set time consuming commands
>> + * should be treated as unsupported.
>> + *
>> + * @return 0 on success, otherwise an error code.
>> + * @return AVERROR(ENOSYS) on unsupported commands.
>> + */
> same I'd prefer to skip this doxy, so I won't have to udpate this when
> the API changes...

Done.

[...]

>> +    if (strcmp(cmd, "tempo") == 0) {
>> +        char   *tail = NULL;
>> +        double tempo = av_strtod(arg,&tail);
>> +
>> +        if (tail&&  *tail) {
>> +            av_log(ctx, AV_LOG_ERROR, "invalid tempo value '%s'\n", arg);
>> +            return AVERROR(EINVAL);
>> +        }
>> +
>> +        return yae_set_tempo(atempo, tempo, ctx);
>> +    }
> you could change the signature of yae_set_tempo to
> static int yae_set_tempo(AVFilterContext *ctx, const char *tempo)
> and factorize some code in init().

Done.

[...]

>> +    .init            =&init,
>> +    .uninit          =&uninit,
>> +    .query_formats   =&query_formats,
>> +    .process_command =&process_command,
> nit: "&" can be skipped

Done.

[...]

>
> BTW add your name as maintainer for this filter in MAINTAINERS if you
> feel like so.

Done.

Thank you for your time,

     Pavel.