[FFmpeg-devel] [PATCH 2/2] avfilter: add sidechaingate filter
Ganesh Ajjanagadde
gajjanag at mit.edu
Tue Dec 1 14:33:51 CET 2015
On Tue, Dec 1, 2015 at 3:50 AM, Paul B Mahol <onemda at gmail.com> wrote:
> On 12/1/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> On Sun, Nov 29, 2015 at 6:03 PM, Paul B Mahol <onemda at gmail.com> wrote:
>>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>>> ---
>>> doc/filters.texi | 61 +++++++++++++++++
>>> libavfilter/Makefile | 1 +
>>> libavfilter/af_agate.c | 170
>>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>> libavfilter/allfilters.c | 1 +
>>> 4 files changed, 230 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/doc/filters.texi b/doc/filters.texi
>>> index 0395c7a..ed4a376 100644
>>> --- a/doc/filters.texi
>>> +++ b/doc/filters.texi
>>> @@ -2531,6 +2531,67 @@ ffmpeg -i main.flac -i sidechain.flac
>>> -filter_complex "[1:a]asplit=2[sc][mix];[0
>>> @end example
>>> @end itemize
>>>
>>> + at section sidechaingate
>>> +
>>> +A sidechain gate acts like a normal (wideband) gate but has the ability
>>> to
>>> +filter the detected signal before sending it to the gain reduction stage.
>>> +Normally a gate uses the full range signal to detect a level above the
>>> +threshold.
>>> +For example: If you cut all lower frequencies from your sidechain signal
>>> +the gate will decrease the volume of your track only if not enough highs
>>> +appear. With this technique you are able to reduce the resonation of a
>>> +natural drum or remove "rumbling" of muted strokes from a heavily
>>> distorted
>>> +guitar.
>>> +It needs two input streams and returns one output stream.
>>> +First input stream will be processed depending on second stream signal.
>>> +
>>> +The filter accepts the following options:
>>> +
>>> + at table @option
>>> + at item level_in
>>> +Set input level before filtering.
>>> +Default is 1. Allowed range is from 0.015625 to 64.
>>> +
>>> + at item range
>>> +Set the level of gain reduction when the signal is below the threshold.
>>> +Default is 0.06125. Allowed range is from 0 to 1.
>>> +
>>> + at item threshold
>>> +If a signal rises above this level the gain reduction is released.
>>> +Default is 0.125. Allowed range is from 0 to 1.
>>> +
>>> + at item ratio
>>> +Set a ratio about which the signal is reduced.
>>> +Default is 2. Allowed range is from 1 to 9000.
>>> +
>>> + at item attack
>>> +Amount of milliseconds the signal has to rise above the threshold before
>>> gain
>>> +reduction stops.
>>> +Default is 20 milliseconds. Allowed range is from 0.01 to 9000.
>>> +
>>> + at item release
>>> +Amount of milliseconds the signal has to fall below the threshold before
>>> the
>>> +reduction is increased again. Default is 250 milliseconds.
>>> +Allowed range is from 0.01 to 9000.
>>> +
>>> + at item makeup
>>> +Set amount of amplification of signal after processing.
>>> +Default is 1. Allowed range is from 1 to 64.
>>> +
>>> + at item knee
>>> +Curve the sharp knee around the threshold to enter gain reduction more
>>> softly.
>>> +Default is 2.828427125. Allowed range is from 1 to 8.
>>> +
>>> + at item detection
>>> +Choose if exact signal should be taken for detection or an RMS like one.
>>> +Default is peak. Can be peak or rms.
>>> +
>>> + at item link
>>> +Choose if the average level between all channels or the louder channel
>>> affects
>>> +the reduction.
>>> +Default is average. Can be average or maximum.
>>> + at end table
>>> +
>>> @section silencedetect
>>>
>>> Detect silence in an audio stream.
>>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>>> index e31bdaa..582fd0e 100644
>>> --- a/libavfilter/Makefile
>>> +++ b/libavfilter/Makefile
>>> @@ -82,6 +82,7 @@ OBJS-$(CONFIG_REPLAYGAIN_FILTER) +=
>>> af_replaygain.o
>>> OBJS-$(CONFIG_RESAMPLE_FILTER) += af_resample.o
>>> OBJS-$(CONFIG_RUBBERBAND_FILTER) += af_rubberband.o
>>> OBJS-$(CONFIG_SIDECHAINCOMPRESS_FILTER) += af_sidechaincompress.o
>>> +OBJS-$(CONFIG_SIDECHAINGATE_FILTER) += af_agate.o
>>> OBJS-$(CONFIG_SILENCEDETECT_FILTER) += af_silencedetect.o
>>> OBJS-$(CONFIG_SILENCEREMOVE_FILTER) += af_silenceremove.o
>>> OBJS-$(CONFIG_STEREOTOOLS_FILTER) += af_stereotools.o
>>> diff --git a/libavfilter/af_agate.c b/libavfilter/af_agate.c
>>> index d3f74fb..23e45c6 100644
>>> --- a/libavfilter/af_agate.c
>>> +++ b/libavfilter/af_agate.c
>>> @@ -18,6 +18,12 @@
>>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>> 02110-1301 USA
>>> */
>>>
>>> +/**
>>> + * @file
>>> + * Audio (Sidechain) Gate filter
>>> + */
>>> +
>>> +#include "libavutil/avassert.h"
>>> #include "libavutil/channel_layout.h"
>>> #include "libavutil/opt.h"
>>> #include "avfilter.h"
>>> @@ -46,12 +52,14 @@ typedef struct AudioGateContext {
>>> double lin_slope;
>>> double attack_coeff;
>>> double release_coeff;
>>> +
>>> + AVFrame *input_frame[2];
>>> } AudioGateContext;
>>>
>>> #define OFFSET(x) offsetof(AudioGateContext, x)
>>> #define A AV_OPT_FLAG_AUDIO_PARAM|AV_OPT_FLAG_FILTERING_PARAM
>>>
>>> -static const AVOption agate_options[] = {
>>> +static const AVOption options[] = {
>>> { "level_in", "set input level", OFFSET(level_in),
>>> AV_OPT_TYPE_DOUBLE, {.dbl=1}, 0.015625, 64, A },
>>> { "range", "set max gain reduction", OFFSET(range),
>>> AV_OPT_TYPE_DOUBLE, {.dbl=0.06125}, 0, 1, A },
>>> { "threshold", "set threshold", OFFSET(threshold),
>>> AV_OPT_TYPE_DOUBLE, {.dbl=0.125}, 0, 1, A },
>>> @@ -69,6 +77,7 @@ static const AVOption agate_options[] = {
>>> { NULL }
>>> };
>>>
>>> +#define agate_options options
>>> AVFILTER_DEFINE_CLASS(agate);
>>>
>>> static int query_formats(AVFilterContext *ctx)
>>> @@ -97,7 +106,7 @@ static int query_formats(AVFilterContext *ctx)
>>> return ff_set_common_samplerates(ctx, formats);
>>> }
>>>
>>> -static int config_input(AVFilterLink *inlink)
>>> +static int agate_config_input(AVFilterLink *inlink)
>>> {
>>> AVFilterContext *ctx = inlink->dst;
>>> AudioGateContext *s = ctx->priv;
>>> @@ -221,7 +230,7 @@ static const AVFilterPad inputs[] = {
>>> .name = "default",
>>> .type = AVMEDIA_TYPE_AUDIO,
>>> .filter_frame = filter_frame,
>>> - .config_props = config_input,
>>> + .config_props = agate_config_input,
>>> },
>>> { NULL }
>>> };
>>> @@ -243,3 +252,158 @@ AVFilter ff_af_agate = {
>>> .inputs = inputs,
>>> .outputs = outputs,
>>> };
>>> +
>>> +#if CONFIG_SIDECHAINGATE_FILTER
>>> +
>>> +#define sidechaingate_options options
>>> +AVFILTER_DEFINE_CLASS(sidechaingate);
>>> +
>>> +static int scfilter_frame(AVFilterLink *link, AVFrame *in)
>>> +{
>>> + AVFilterContext *ctx = link->dst;
>>> + AudioGateContext *s = ctx->priv;
>>> + AVFilterLink *outlink = ctx->outputs[0];
>>> + const double *scsrc;
>>> + double *sample;
>>> + int nb_samples;
>>> + int ret, i;
>>> +
>>> + for (i = 0; i < 2; i++)
>>> + if (link == ctx->inputs[i])
>>> + break;
>>> + av_assert0(i < 2 && !s->input_frame[i]);
>>> + s->input_frame[i] = in;
>>> +
>>> + if (!s->input_frame[0] || !s->input_frame[1])
>>> + return 0;
>>> +
>>> + nb_samples = FFMIN(s->input_frame[0]->nb_samples,
>>> + s->input_frame[1]->nb_samples);
>>> +
>>> + sample = (double *)s->input_frame[0]->data[0];
>>> + scsrc = (const double *)s->input_frame[1]->data[0];
>>> +
>>> + gate(s, sample, sample, scsrc, nb_samples,
>>> + ctx->inputs[0], ctx->inputs[1]);
>>> + ret = ff_filter_frame(outlink, s->input_frame[0]);
>>> +
>>> + s->input_frame[0] = NULL;
>>> + av_frame_free(&s->input_frame[1]);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int screquest_frame(AVFilterLink *outlink)
>>> +{
>>> + AVFilterContext *ctx = outlink->src;
>>> + AudioGateContext *s = ctx->priv;
>>> + int i, ret;
>>> +
>>> + /* get a frame on each input */
>>> + for (i = 0; i < 2; i++) {
>>> + AVFilterLink *inlink = ctx->inputs[i];
>>> + if (!s->input_frame[i] &&
>>> + (ret = ff_request_frame(inlink)) < 0)
>>> + return ret;
>>> +
>>> + /* request the same number of samples on all inputs */
>>> + if (i == 0)
>>> + ctx->inputs[1]->request_samples =
>>> s->input_frame[0]->nb_samples;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int scquery_formats(AVFilterContext *ctx)
>>> +{
>>> + AVFilterFormats *formats;
>>> + AVFilterChannelLayouts *layouts = NULL;
>>> + static const enum AVSampleFormat sample_fmts[] = {
>>> + AV_SAMPLE_FMT_DBL,
>>> + AV_SAMPLE_FMT_NONE
>>> + };
>>> + int ret, i;
>>> +
>>> + if (!ctx->inputs[0]->in_channel_layouts ||
>>> + !ctx->inputs[0]->in_channel_layouts->nb_channel_layouts) {
>>> + av_log(ctx, AV_LOG_WARNING,
>>> + "No channel layout for input 1\n");
>>> + return AVERROR(EAGAIN);
>>> + }
>>> +
>>> + if ((ret = ff_add_channel_layout(&layouts,
>>> ctx->inputs[0]->in_channel_layouts->channel_layouts[0])) < 0 ||
>>> + (ret = ff_channel_layouts_ref(layouts,
>>> &ctx->outputs[0]->in_channel_layouts)) < 0)
>>> + return ret;
>>> +
>>> + for (i = 0; i < 2; i++) {
>>> + layouts = ff_all_channel_counts();
>>> + if ((ret = ff_channel_layouts_ref(layouts,
>>> &ctx->inputs[i]->out_channel_layouts)) < 0)
>>> + return ret;
>>> + }
>>> +
>>> + formats = ff_make_format_list(sample_fmts);
>>> + if ((ret = ff_set_common_formats(ctx, formats)) < 0)
>>> + return ret;
>>> +
>>> + formats = ff_all_samplerates();
>>> + return ff_set_common_samplerates(ctx, formats);
>>
>> Please fix the memleak issue - similar such things have been pointed
>> out for previous reviews by me. Please repost - you have ignored this
>> point in the past: see commit ID
>> 3f895dcb0dcbcbf10a621bf4bfae6d8879899015 which you pushed recently, in
>> spite of the issue being pointed out in review:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2015-November/183861.html.
>>
>> This does not seem cool to me. What is even worse is that this is
>> something that flags Coverity (legitimately AFAIK), see a patch I have
>> posted: https://ffmpeg.org/pipermail/ffmpeg-devel/2015-November/183335.html.
>>
>> I do not mind fixing broken code that went past review, but I do mind
>> when it is recognized, ignored during patch review, and could have
>> easily been avoided with a 5 minute effort addressing the comment.
>>
>> If this continues to be ignored, I am going to find a way to mark
>> filters as "experimental" and send out a patch, similar to
>> encoders/decoders. Users should know about problematic filters like
>> this that have easily recognized problems.
>
> Mark something, that leaks on error that never happens, as experimental
> sounds unfair to me.
Prove to me that "it does not happen". I admit it does rarely do so in
practice, nevertheless it must be addressed for completeness. However,
the "experimental" comment is really not for this per se. I made this
comment simply because it was an issue pointed out in review that you
failed to address in any way (not even with a simple one-liner saying
"it does not happen in practice, so I don't care"), and simply pushed
into master. This was just an illustration of it that I noticed once I
started reviewing some filters posted by you.
You have called me out in the past for pushing without approval:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/182511.html,
which in fact was not even true as they had been approved. This is far
worse IMO, since the proposed code is actually defective and the
defect has been pointed out during review.
I appreciate all your hard work and very good effort in getting new
filters into FFmpeg. What I don't like is your lax standard for
getting things into the master repo. Unless you start addressing such
things, I do not consider such a move unfair. All you need to do is
spend 5-10 minutes of your time per filter actually addressing things
pointed out by reviewers. That is all I ask.
>
> Anyway, shouldn't this be called in one place instead of hundreds in each
> filter?
Then create an API for this if you don't like the way it is being
done. Until such a thing is done, the issue stands, and must be
addressed.
>
> This all is really very low hanging fruit to me.
Since you are stating an opinion here, I will state mine below.
So what? I would go so far to say that pulling in filters into FFmpeg
that already exist in the wild but can't be integrated naturally into
FFmpeg until some trivial massaging of the code is done is "low
hanging fruit". Many filters integrated by you are of that nature. In
fact, I now understand where your "bit exact" concerns come from - you
simply want to match an existing filter (of course there is nothing
wrong with that per se). On this note, I freely admit the work I do is
extremely low hanging fruit. In fact, most human activity is low
hanging and incremental in nature, we all "stand on the shoulders of
giants".
That does not change what should be done. A reviewer spends time and
effort to improve something; you fail to address the reviewer.
>
>>
>> BTW (for all FFmpeg users out there though this may be well known),
>> but for completeness I remark that avfilter tends to be far worse in
>> Coverity than the other components. It is not a surprise at all given
>> incidents like this. I entirely sympathize with wm4 regarding the
>> quality of filters entering the repo.
>>
>>> +}
>>> +
>>> +static int scconfig_output(AVFilterLink *outlink)
>>> +{
>>> + AVFilterContext *ctx = outlink->src;
>>> +
>>> + if (ctx->inputs[0]->sample_rate != ctx->inputs[1]->sample_rate) {
>>> + av_log(ctx, AV_LOG_ERROR,
>>> + "Inputs must have the same sample rate "
>>> + "%d for in0 vs %d for in1\n",
>>> + ctx->inputs[0]->sample_rate, ctx->inputs[1]->sample_rate);
>>> + return AVERROR(EINVAL);
>>> + }
>>> +
>>> + outlink->sample_rate = ctx->inputs[0]->sample_rate;
>>> + outlink->time_base = ctx->inputs[0]->time_base;
>>> + outlink->channel_layout = ctx->inputs[0]->channel_layout;
>>> + outlink->channels = ctx->inputs[0]->channels;
>>> +
>>> + agate_config_input(ctx->inputs[0]);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const AVFilterPad sidechaingate_inputs[] = {
>>> + {
>>> + .name = "main",
>>> + .type = AVMEDIA_TYPE_AUDIO,
>>> + .filter_frame = scfilter_frame,
>>> + .needs_writable = 1,
>>> + .needs_fifo = 1,
>>> + },{
>>> + .name = "sidechain",
>>> + .type = AVMEDIA_TYPE_AUDIO,
>>> + .filter_frame = scfilter_frame,
>>> + .needs_fifo = 1,
>>> + },
>>> + { NULL }
>>> +};
>>> +
>>> +static const AVFilterPad sidechaingate_outputs[] = {
>>> + {
>>> + .name = "default",
>>> + .type = AVMEDIA_TYPE_AUDIO,
>>> + .config_props = scconfig_output,
>>> + .request_frame = screquest_frame,
>>> + },
>>> + { NULL }
>>> +};
>>> +
>>> +AVFilter ff_af_sidechaingate = {
>>> + .name = "sidechaingate",
>>> + .description = NULL_IF_CONFIG_SMALL("Audio sidechain gate."),
>>> + .priv_size = sizeof(AudioGateContext),
>>> + .priv_class = &sidechaingate_class,
>>> + .query_formats = scquery_formats,
>>> + .inputs = sidechaingate_inputs,
>>> + .outputs = sidechaingate_outputs,
>>> +};
>>> +#endif /* CONFIG_SIDECHAINGATE_FILTER */
>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>>> index ccd3f35..9c3778d 100644
>>> --- a/libavfilter/allfilters.c
>>> +++ b/libavfilter/allfilters.c
>>> @@ -104,6 +104,7 @@ void avfilter_register_all(void)
>>> REGISTER_FILTER(RESAMPLE, resample, af);
>>> REGISTER_FILTER(RUBBERBAND, rubberband, af);
>>> REGISTER_FILTER(SIDECHAINCOMPRESS, sidechaincompress, af);
>>> + REGISTER_FILTER(SIDECHAINGATE, sidechaingate, af);
>>> REGISTER_FILTER(SILENCEDETECT, silencedetect, af);
>>> REGISTER_FILTER(SILENCEREMOVE, silenceremove, af);
>>> REGISTER_FILTER(STEREOTOOLS, stereotools, af);
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list