[FFmpeg-devel] [GSoC] Qualification Task
Peter Ross
pross at xvid.org
Tue Mar 22 11:28:10 CET 2011
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.
> The attached patch is against the audio-filters branch from sastes-ffmpeg repo.
> 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.
>
> The segfaults mentioned on IRC were actually because I was clearing out of
> bounds memory and overwriting open_inputs and open_outputs in
> configure_audio_filters, so a bunch of string utils where segfaulting on the
> NULL names and whatnot :D my bad.
> diff --git a/libavfilter/af_widen.c b/libavfilter/af_widen.c
> new file mode 100644
> index 0000000..c0bc9ff
> --- /dev/null
> +++ b/libavfilter/af_widen.c
> @@ -0,0 +1,126 @@
Please include a license header at the top of the file. libsox is LGPL.
> +/* Adapted by Mina Nagy (mnzaki at gmail.com) from the libSoX earwax effect.
> + */
> +
> +/**
> + * @file
> + * Stereo Widening Effect. Adds audio cues to move stereo image in
> + * front of the listener.
> + */
> +
> +#include "avfilter.h"
> +#include "libavcore/audioconvert.h"
Do you use anything for audioconvert?
> +
> +static const int8_t filt[32 * 2] = {
move the NUMTAPS define up higher and use 'static const int8_t filt[NUMTAPS] ..'
> +/* 30?? 330?? */
those question marks should say 'degrees'
> + 4, -6, /* 32 tap stereo FIR filter. */
> + 4, -11, /* One side filters as if the */
> + -1, -5, /* signal was from 30 degrees */
> + 3, 3, /* from the ear, the other as */
> + -2, 5, /* if 330 degrees. */
> + -5, 0, /* from the sox earwax filter */
why not cut and paste the ASCII art from the sox file?
> + 9, 1,
> + 6, 3,
> + -4, -1,
> + -5, -3,
> + -2, -5,
> + -7, 1,
> + 6, -7,
> + 30, -29,
> + 12, -3,
> + -11, 4,
> + -3, 7,
> + -20, 23,
> + 2, 0,
> + 1, -6,
> + -14, -5,
> + 15, -18,
> + 6, 7,
> + 15, -10,
> + -14, 22,
> + -7, -2,
> + -4, 9,
> + 6, -12,
> + 6, -6,
> + 0, -11,
> + 0, -5,
> + 4, 0
> +};
> +
> +#define NUMTAPS 64
> +typedef struct {
> + int16_t taps[NUMTAPS * 2];
> +} WidenContext;
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> +{
> + WidenContext *widen = ctx->priv;
> + memset(widen->taps, 0, sizeof(widen->taps));
> + return 0;
> +}
the init() function is not neccessary, as libavfilter zeroises the priv buffer
for your automatically.
> +static int query_formats(AVFilterContext *ctx)
> +{
> + AVFilterFormats *formats = NULL;
> + avfilter_add_format(&formats, AV_SAMPLE_FMT_S16);
> + avfilter_formats_ref(formats, &ctx->inputs[0]->out_formats);
> + avfilter_formats_ref(formats, &ctx->outputs[0]->in_formats);
> + return 0;
> +}
> +
> +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){
> + 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
> + 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)
> + .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?
Cheers,
-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110322/1086b4d6/attachment.asc>
More information about the ffmpeg-devel
mailing list