[FFmpeg-devel] [PATCH] libavfilter: added atempo filter (revised patch)
Pavel Koshevoy
pkoshevoy at gmail.com
Fri Jun 8 06:59:13 CEST 2012
On 06/07/2012 12:00 AM, Clément Bœsch wrote:
> On Wed, Jun 06, 2012 at 10:17:28AM -0600, pkoshevoy at gmail.com wrote:
>> From: Pavel Koshevoy<pkoshevoy at gmail.com>
>>
[...]
>> -
> Please keep that empty line :)
done
[...]
> +The filter accepts exactly one parameter, the audio tempo. If not
> +specified then the filter will assume nominal tempo.
> add "(a value of 1.0)"?
changed to "will assume nominal 1.0 tempo"
[...]
> +typedef enum {
> + kLoadFragment = 0,
> + kAdjustPosition = 1,
> + kReloadFragment = 2,
> + kOutputOverlapAdd = 3,
> + kFlushOutput = 4
> +
> nit: you can add a comma at the end of this line so adding new entries
> won't require a modification of this line.
done, and I also dropped the k prefix since it raised some eyebrows.
[...]
> + unsigned char *buffer;
> +
> Please s/unsigned char/uint8_t/ all over the file
done, and I also replaced short int with int16_t and unsigned int with uint32_t
[...]
> +
> + // for managing AVFilterPad::request_frame and AVFilterPad::filter_samples
> nit: AVFilterPad.request_frame and AVFilterPad.filter_samples
done, although it looks strange to me. Is :: scope operator not part of C language?
>> +static void yae_constructor(ATempoContext *atempo)
>> +{
>> + 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;
>> +}
>> +
> This context should already be zero-ed, so it's likely this function can
> be simplified to:
>
> atempo->format = AV_SAMPLE_FMT_NONE;
> atempo->tempo = 1.0;
> atempo->state = kLoadFragment;
As a rule I do not rely on someone else to initialize my data members for me --
it hurts reuse-ability. However, since you have assured me that ffmpeg will
memset the context private data to 0 I have removed the redundant
initialization. Also, I got rid of yae_constructor and yae_destructor and moved
relevant code to init and uninit instead.
[...]
>> + if (atempo->fft_inverse) {
>> + av_fft_end(atempo->fft_inverse);
>> + atempo->fft_inverse = NULL;
>> + }
>> +
> You don't need to check for fft_forward and fft_inverse, av_fft_end() will
> do it.
done
[...]
>> + if (atempo->dst_buffer) {
>> + avfilter_unref_buffer(atempo->dst_buffer);
>> + atempo->dst_buffer = NULL;
> avfilter_unref_bufferp()
done
[...]
>> + // adjust window size to be a power-of-two integer:
>> + nlevels = av_log2_c(atempo->window);
> av_log2()
done
[...]
>> + atempo->frag[1].xdat = av_realloc(atempo->frag[1].xdat,
>> + atempo->window * 2 *
>> + sizeof(FFTComplex));
>> +
> Here and below, it would be nice to check for the realloc and raise AVERROR(ENOMEM) in
> case of error.
done
[...]
>> + atempo->correlation = (FFTComplex *)av_realloc(atempo->correlation,
>> + atempo->window * 2 *
>> + sizeof(FFTComplex));
> You don't need that cast in C.
done
[...]
>> + for (int i = 0; i< atempo->window; i++) {
> I think we still try to avoid declaring the "int" in the loop for some
> compatibility issues.
done
[...]
>> +/**
>> + * Find the minimum of two scalars
>> + */
>> +#define yae_min(TScalar, a, b) \
>> + ((TScalar)a< (TScalar)b ? \
>> + (TScalar)a : \
>> + (TScalar)b)
>> +
>> +/**
>> + * Find the maximum of two scalars
>> + */
>> +#define yae_max(TScalar, a, b) \
>> + ((TScalar)a< (TScalar)b ? \
>> + (TScalar)b : \
>> + (TScalar)a)
>> +
> Is the cast really needed?
>
> Also, you can use FFMIN() and FFMAX()
Because this code was originally developed and tested in C++, yae_min and
yae_max were reproducing std::min<T>(const T & a, const T & b) and
std::max<T>(..) interface and behavior. I wanted to minimize untested changed.
However, I was able to substitute FFMIN and FFMAX instead.
>> +
>> +/**
>> + * A helper macro for initializing complex data buffer with scalar data
>> + * of a given type.
>> + */
>> +#define yae_init_xdat(TScalar, scalar_max) \
>> + do { \
>> + const unsigned char *src_end = \
>> + src + frag->nsamples * atempo->channels * sizeof(TScalar); \
>> + \
>> + FFTComplex *xdat = frag->xdat; \
>> + TScalar tmp; \
>> + \
>> + if (atempo->channels == 1) { \
>> + float s; \
>> + \
>> + for (; src< src_end; blend++) { \
>> + memcpy(&tmp, src, sizeof(TScalar)); \
>> + src += sizeof(TScalar); \
>> + \
> tmp = *src++ is not possible?
No, because tmp is of type TScalar (whatever that may be), and src is of type
unsigned char *.
However, I've modified the memcpy(..) line to be tmp = *(const TScalar *)src
instead.
[...]
>
>> + for (int i = 1; i< atempo->channels; i++) { \
> ditto int
done
[...]
> Wouldn't it be simpler to request for planar formats? (U16P, S16P, etc)
No actually, planar data is not easily stored in a ring buffer, unless a
separate buffer is used for each channel. I've added a comment to the
query_formats(..) function mentioning that planar formats are explicitly not
supported.
[...]
> [...]
>> + if (atempo->dst_buffer) {
>> + avfilter_unref_buffer(atempo->dst_buffer);
>> + atempo->dst_buffer = NULL;
> avfilter_unref_bufferp()
done
[...]
>> #define LIBAVFILTER_VERSION_MAJOR 2
>> #define LIBAVFILTER_VERSION_MINOR 78
>> -#define LIBAVFILTER_VERSION_MICRO 100
>> +#define LIBAVFILTER_VERSION_MICRO 101
>>
> I think you need to bump MINOR instead.
I've removed my changes from libavfilter/version.h
I have already submitted another patch incorporating the requested changes.
Thank you,
Pavel.
More information about the ffmpeg-devel
mailing list