On 12/1/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote:
> On Tue, Dec 1, 2015 at 3:50 AM, Paul B Mahol <one...@gmail.com> wrote:
>> On 12/1/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote:
>>> On Sun, Nov 29, 2015 at 6:03 PM, Paul B Mahol <one...@gmail.com> wrote:
>>>> Signed-off-by: Paul B Mahol <one...@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
>>>>
>>>> +@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:
>>>> +
>>>> +@table @option
>>>> +@item level_in
>>>> +Set input level before filtering.
>>>> +Default is 1. Allowed range is from 0.015625 to 64.
>>>> +
>>>> +@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.
>>>> +
>>>> +@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.
>>>> +
>>>> +@item ratio
>>>> +Set a ratio about which the signal is reduced.
>>>> +Default is 2. Allowed range is from 1 to 9000.
>>>> +
>>>> +@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.
>>>> +
>>>> +@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.
>>>> +
>>>> +@item makeup
>>>> +Set amount of amplification of signal after processing.
>>>> +Default is 1. Allowed range is from 1 to 64.
>>>> +
>>>> +@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.
>>>> +
>>>> +@item detection
>>>> +Choose if exact signal should be taken for detection or an RMS like
>>>> one.
>>>> +Default is peak. Can be peak or rms.
>>>> +
>>>> +@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.
>>>> +@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.
I don't think API is needed for this. Just one call in right place IIRC.
There is more than 50 audio filter that needs to be updated to address this?
>
>>
>> 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@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
