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

[Muhammad Faiz]

Thank's for the suggestion.
I will review.


On Wed, May 28, 2014 at 8:40 AM, Muhammad Faiz <mfcc64 at gmail.com> wrote:

> Thank's for the suggestion.
> I will review.
>
>
> On Mon, May 26, 2014 at 10:51 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
>
>> 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
>>
>
>