[FFmpeg-devel] [GSoC] Qualification Task

[Stefano Sabatini]

On date Tuesday 2011-03-22 21:28:10 +1100, Peter Ross encoded:
> On Tue, Mar 22, 2011 at 09:54:02AM +0200, Mina Nagy Zaki wrote:
> > Hi,
> > I've ported a small filter from sox, the 'earwax' filter, essentially a stereo 
> > widening effect. It's a relatively simple filter, I was actually looking to 
> > write a general sox wrapper but turns out there would be a few things that 
> > would make this slightly difficult.
> 
> Can you elaborate on this, just curious.

I remeber the mail from Hemanth, he said that there was the need to
use threading for it. I have the last patch from Hemanth, and the
mails with the discussion, and I can ask Hemanth to comment on this.

> > The attached patch is against the audio-filters branch from sastes-ffmpeg repo.

Link for lazy people:
http://gitorious.org/~saste/ffmpeg/sastes-ffmpeg/commits/20110217-audio-filters

(note, it is against the old ffmpeg.git.org repo, main difference with
current trunk is the av_samples_* API interface).

> > For reference, the SoX effect code: 
> > http://sox.git.sourceforge.net/git/gitweb.cgi?p=sox/sox;a=blob_plain;f=src/earwax.c;hb=HEAD
> > 
> > I've actually changed the main loop ('flow' function in earwax.c) to avoid what 
> > looks like a bug in the sox implementation (although it could turn out to be a 
> > trick to add delay, but the delay would be dependent on buffer size :S). The 
> > output seems pretty similar to my ears though.
> > It's not a very good effect BTW :D
> 
> Great job. Patch review below.
> 

> > I've come across a bug in graph string parsing, when there's only one filter 
> > and you use a semicolon like -af "widen;" it works without the semicolon 
> > though.

ffplay IN -vf null;

works fine here. Can you elaborate on this?

[...]
> > +static void filter_samples(AVFilterLink *inlink, AVFilterBufferRef *insamples)
> > +{
> > +    int16_t *taps = ((WidenContext*)inlink->dst->priv)->taps;
> > +    int16_t *in, *endin, *out;
> > +    int32_t sample;
> > +    int j;
> > +    AVFilterBufferRef *outref =
> > +        avfilter_get_audio_buffer(inlink, AV_PERM_WRITE,
> > +                                  inlink->format,
> > +                                  insamples->audio->nb_samples,
> > +                                  inlink->channel_layout,
> > +                                  insamples->audio->planar);
> > +    out = outref->data[0];
> > +    // First copy part of the new input into the tap memory
> > +    // and process the previously saved taps
> > +    memcpy(taps+NUMTAPS, insamples->data[0], NUMTAPS * sizeof(int16_t));
> 
> > +    in = taps;
> > +    endin = in + NUMTAPS;
> 
> might be more obvious to use 'endin = taps + NUMTAPS'
> also pleae align, such that the equals signs line up. e.g.
> 
>     in    = taps;
>     endin = taps + NUMTAPS;
> 

> > +    while(in < endin){

Nit+:
        while_(in < endian) {
            for_(...)

here and below.

> > +        sample = 0;
> > +        for(j = 0; j < NUMTAPS; j++)
> > +            sample += in[j] * filt[j];
> > +        *out = sample >> 6;
> > +        out++; in++;
> > +    }
> > +
> > +    in = insamples->data[0];
> > +    endin = in + insamples->audio->nb_samples * 2 - NUMTAPS;
> 
> allign
> 
> > +    while(in < endin){
> > +        sample = 0;
> > +        for(j = 0; j < NUMTAPS; j++)
> > +            sample += in[j] * filt[j];
> > +        *out = sample >> 6;
> > +        out++; in++;
> > +    }
> 
> the above two while loops are identical
> 

> they also look a lot like scalarproduct_int16(), which can be found in DSPContext

I note that DSP is not yet used in lavfi, it may be a good idea to
move it to lavu, for the moment we can require a conditional
dependency of the filters using it on lavc.

> > +    memcpy(taps, in, NUMTAPS * sizeof(int16_t));
> > +
> > +    avfilter_filter_samples(inlink->dst->outputs[0], outref);
> > +    avfilter_unref_buffer(insamples);
> > +}
> > +
> > +AVFilter avfilter_af_widen = {
> > +    .name = "widen",
> > +    .description = NULL_IF_CONFIG_SMALL("Widen the stereo image."),
> 
> consider dropping the period at the end of the description. (i dont think
> it want meant to be a full english sentence)

Nit+: the dot can stay afterall, that's how all the other descriptions
are formatted.

> > +    .init = init,
> > +    .query_formats = query_formats,
> > +    .priv_size = sizeof(WidenContext),
> > +    .inputs = (AVFilterPad[])   {{  .name           = "default",
> > +                                    .type           = AVMEDIA_TYPE_AUDIO,
> > +                                    .filter_samples = filter_samples,
> > +                                    .min_perms      = AV_PERM_READ, },
> > +                                 {  .name = NULL  }},
> > +
> > +    .outputs = (AVFilterPad[])  {{  .name           = "default",
> > +                                    .type           = AVMEDIA_TYPE_AUDIO, },
> > +                                 {  .name = NULL}},
> > +};
> 
> Please align these also.
> 
> What happens if this filter is applied to non-stereo stream?

Possibly won't work. As I understand it, this will need some form of
negotiation for the audio channel layout.

BTW, Peter would you co-mentor this project?, that would be much
appreciated.
-- 
FFmpeg = Frightening Freaking Mythic Ponderous Extroverse Gadget