[FFmpeg-devel] [PATCH][BULK] new multimedia filter avf_showcqt.c

[Michael Niedermayer]

Hi

On Sun, May 25, 2014 at 07:24:07PM -0700, Muhammad Faiz wrote:
> Hi,
> I'm sorry, I send this again
> this filter is same as showspectrum but with constant Q transform,
> so frequency is spaced logarithmically

[...]

> +
> +/* image 'A BC D EF G ' from font 'Nimbus Mono L, Bold' */
> +static uint8_t abcdefg_string[] = {

see libavutil/xga_font_data*
note if you prefer your font, dont hesitate to add it there


[...]


> +typedef struct {
> +    const AVClass *class;
> +    AVFrame *outpicref;
> +    FFTContext *fft_context;
> +    FFTComplex *fft_data;
> +    FFTComplex *fft_result_left;
> +    FFTComplex *fft_result_right;
> +    SparseCoeff *coeffs[VIDEO_WIDTH];
> +    int coeffs_len[VIDEO_WIDTH];
> +    uint8_t spectogram[SPECTOGRAM_HEIGHT][VIDEO_WIDTH][3];
> +    int64_t frame_count;
> +    int spectogram_count;
> +    int spectogram_index;
> +    int fft_bits;
> +    int req_fullfilled;
> +    int remaining_fill;
> +    

trailing whitespace is rejected by our git repository


> +    /* options */
> +    double volume;
> +    double timeclamp;
> +    float coeffclamp;
> +    float gamma;

> +    int fps;

fps should probably be a AVRational


[...]

> +static int config_output(AVFilterLink *outlink)
> +{
> +    AVFilterContext *ctx = outlink->src;
> +    AVFilterLink *inlink = ctx->inputs[0];
> +    ShowCQTContext *s = ctx->priv;
> +    SparseCoeff *coeff_sort;
> +    int fft_len, k;
> +    int num_coeffs = 0;
> +    int rate = inlink->sample_rate;
> +    double max_len = rate * (double) s->timeclamp;
> +    s->fft_bits = ceil(log2(max_len));
> +    fft_len = 1 << s->fft_bits;
> +    
> +    if((rate % (s->fps * s->count)))
> +    {
> +        av_log(ctx, AV_LOG_ERROR, "Rate (%u) is not divisible by fps*count (%u*%u)\n", rate, s->fps, s->count);
> +        return AVERROR(EINVAL);
> +    }
> +    

> +    s->fft_data = av_malloc(sizeof(FFTComplex)*fft_len);

please use av_malloc_array()
this ensures integer overflows in the multiplications wont occur
and lead to unexpected results


> +    if (!s->fft_data)
> +        return AVERROR(ENOMEM);

> +    coeff_sort = (SparseCoeff*) s->fft_data;

i suspect this is a aliasing violation and the later code has
undefined behavior, its probably not even guranteed that the 2 structs
have teh same size


[...]
> +    av_log(ctx, AV_LOG_INFO, "Calculating spectral kernel, please wait\n");
> +    for (k = 0; k < VIDEO_WIDTH; k++)
> +    {
> +        int x, hlen = fft_len >> 1;
> +        float total = 0;
> +        float partial = 0;
> +        double freq = BASE_FREQ * exp2(k * (1.0/192.0));
> +        double tlen = rate * (24.0 * 16.0) /freq;
> +        tlen = tlen * max_len / (tlen + max_len);
> +        s->fft_data[0].re = 0;
> +        s->fft_data[0].im = 0;
> +        s->fft_data[hlen].re = window_function(0) * (1.0/tlen) * s->volume * (1.0/fft_len);
> +        s->fft_data[hlen].im = 0;
> +        for (x = 1; x < hlen; x++)
> +        {
> +            double sv, cv, w;

> +            sincos(2.0*M_PI*freq*x*(1.0/rate), &sv, &cv);

sincos is not portable


[...]
> +        /* drawing font */
> +        {
> +            static float color[VIDEO_WIDTH];
> +            static float alpha[FONT_HEIGHT][VIDEO_WIDTH];
> +            static int initialized = 0;
> +            int x, y;
> +            /* precomputing */
> +            if (!initialized)
> +            {
> +                for (x = 0; x < VIDEO_WIDTH; x++)
> +                {
> +                    if((x >= (12*3+8)*16) && (x < (12*4+8)*16))
> +                    {
> +                        float fx = (x - (12*3+8)*16) * (1.0f/192.0f);
> +                        float s = sinf(M_PI*fx);
> +                        color[x] = s*s*255.0f; 
> +                    }
> +                    else
> +                        color[x] = 0.0f;
> +                    
> +                    for (y = 0; y < FONT_HEIGHT; y++)
> +                    {
> +                        int u = (x + 7*16) % (12*16);
> +                        alpha[y][x] = (1.0f/255.0f) * abcdefg_string[y*(12*16) + u];
> +                    }
> +                }
> +                initialized = 1;
> +            }

its probably better to do this at init/config time
and maybe make initialized, volatile or do the whole uncondiotional
at init time if you want to be really sure it cant be hit by some
race and out of order mem accesses

[...]

also documentation is missing

thanks

PS: this filter is very cool looking

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140526/75af6e38/attachment.asc>