[FFmpeg-devel] [PATCH 1/5] lavfi: remplace passthrough_filter_frame with a flag.

Stefano Sabatini stefasab at gmail.com
Sat May 11 16:50:36 CEST 2013


On date Saturday 2013-05-11 15:19:06 +0200, Stefano Sabatini encoded:
> On date Friday 2013-05-10 17:49:46 +0200, Clément Bœsch encoded:
> > On Thu, May 09, 2013 at 01:53:23AM +0200, Clément Bœsch wrote:
> > > With the introduction of AVFilterContext->is_disabled, we can simplify
> > > the custom passthrough mode in filters.
> > > ---
> > >  libavfilter/af_apad.c    |  3 +--
> > >  libavfilter/avfilter.c   |  6 +++---
> > >  libavfilter/avfilter.h   | 19 ++++++-------------
> > >  libavfilter/version.h    |  4 ++--
> > >  libavfilter/vf_overlay.c | 25 ++++---------------------
> > >  5 files changed, 16 insertions(+), 41 deletions(-)
> > > 
> > 
> > New version attached.
> > 
> > -- 
> > Clément B.
> 
> > From b0c4e23be426386a2fb6a1a974a5fff751c58e2d Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
> > Date: Thu, 9 May 2013 01:04:41 +0200
> > Subject: [PATCH 1/5] lavfi: replace passthrough_filter_frame with a flag.
> > 
> > With the introduction of AVFilterContext->is_disabled, we can simplify
> > the custom passthrough mode in filters.
> > 
> > This commit is technically a compat break, but the timeline was
> > introduced very recently.
> > ---
> >  libavfilter/af_apad.c              |  3 +--
> >  libavfilter/af_volume.c            |  2 +-
> >  libavfilter/avfilter.c             |  6 +++---
> >  libavfilter/avfilter.h             | 26 ++++++++++++--------------
> >  libavfilter/version.h              |  4 ++--
> >  libavfilter/vf_boxblur.c           |  2 +-
> >  libavfilter/vf_colorbalance.c      |  2 +-
> >  libavfilter/vf_colorchannelmixer.c |  2 +-
> >  libavfilter/vf_colormatrix.c       |  2 +-
> >  libavfilter/vf_cropdetect.c        |  2 +-
> >  libavfilter/vf_curves.c            |  2 +-
> >  libavfilter/vf_delogo.c            |  2 +-
> >  libavfilter/vf_drawbox.c           |  2 +-
> >  libavfilter/vf_edgedetect.c        |  2 +-
> >  libavfilter/vf_gradfun.c           |  2 +-
> >  libavfilter/vf_histeq.c            |  2 +-
> >  libavfilter/vf_hue.c               |  2 +-
> >  libavfilter/vf_lut.c               |  2 +-
> >  libavfilter/vf_noise.c             |  2 +-
> >  libavfilter/vf_overlay.c           | 25 ++++---------------------
> >  libavfilter/vf_pp.c                |  2 +-
> >  libavfilter/vf_removelogo.c        |  2 +-
> >  libavfilter/vf_smartblur.c         |  2 +-
> >  libavfilter/vf_unsharp.c           |  2 +-
> >  24 files changed, 41 insertions(+), 61 deletions(-)
> > 
> > diff --git a/libavfilter/af_apad.c b/libavfilter/af_apad.c
> > index 265b76a..c863792 100644
> > --- a/libavfilter/af_apad.c
> > +++ b/libavfilter/af_apad.c
> > @@ -131,7 +131,6 @@ static const AVFilterPad apad_inputs[] = {
> >          .name         = "default",
> >          .type         = AVMEDIA_TYPE_AUDIO,
> >          .filter_frame = filter_frame,
> > -        .passthrough_filter_frame = filter_frame,
> >      },
> >      { NULL },
> >  };
> > @@ -153,5 +152,5 @@ AVFilter avfilter_af_apad = {
> >      .inputs        = apad_inputs,
> >      .outputs       = apad_outputs,
> >      .priv_class    = &apad_class,
> > -    .flags         = AVFILTER_FLAG_SUPPORT_TIMELINE,
> > +    .flags         = AVFILTER_FLAG_INTERNAL_TIMELINE,
> >  };
> > diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c
> > index b7aec8d..79054af 100644
> > --- a/libavfilter/af_volume.c
> > +++ b/libavfilter/af_volume.c
> > @@ -296,5 +296,5 @@ AVFilter avfilter_af_volume = {
> >      .init           = init,
> >      .inputs         = avfilter_af_volume_inputs,
> >      .outputs        = avfilter_af_volume_outputs,
> > -    .flags          = AVFILTER_FLAG_SUPPORT_TIMELINE,
> > +    .flags          = AVFILTER_FLAG_GENERIC_TIMELINE,
> >  };
> > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > index 3f94dde..6676a58 100644
> > --- a/libavfilter/avfilter.c
> > +++ b/libavfilter/avfilter.c
> > @@ -995,9 +995,9 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
> >          dstctx->var_values[VAR_POS] = pos == -1 ? NAN : pos;
> >  
> >          dstctx->is_disabled = !av_expr_eval(dstctx->enable, dstctx->var_values, NULL);
> > -        if (dstctx->is_disabled)
> > -            filter_frame = dst->passthrough_filter_frame ? dst->passthrough_filter_frame
> > -                                                         : default_filter_frame;
> > +        if (dstctx->is_disabled &&
> > +            (dstctx->filter->flags & AVFILTER_FLAG_GENERIC_TIMELINE))
> > +            filter_frame = default_filter_frame;
> >      }
> >      ret = filter_frame(link, out);
> >      link->frame_count++;
> > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > index a79c86c..8695a36 100644
> > --- a/libavfilter/avfilter.h
> > +++ b/libavfilter/avfilter.h
> > @@ -385,19 +385,6 @@ struct AVFilterPad {
> >      int needs_fifo;
> >  
> >      int needs_writable;
> > -
> > -    /**
> > -     * Passthrough filtering callback.
> > -     *
> > -     * If a filter supports timeline editing (in case
> > -     * AVFILTER_FLAG_SUPPORT_TIMELINE is enabled) then it can implement a
> > -     * custom passthrough callback to update its local context (for example to
> > -     * keep a frame reference, or simply send the filter to a custom outlink).
> > -     * The filter must not do any change to the frame in this callback.
> > -     *
> > -     * Input pads only.
> > -     */
> > -    int (*passthrough_filter_frame)(AVFilterLink *link, AVFrame *frame);
> >  };
> >  #endif
> >  
> > @@ -446,7 +433,18 @@ enum AVMediaType avfilter_pad_get_type(const AVFilterPad *pads, int pad_idx);
> >   * to enable or disable a filter in the timeline. Filters supporting this
> >   * option have this flag set.
> >   */
> > -#define AVFILTER_FLAG_SUPPORT_TIMELINE      (1 << 16)
> 
> > +#define AVFILTER_FLAG_GENERIC_TIMELINE      (1 << 16)
> > +/**
> 
> > + * Same as AVFILTER_FLAG_GENERIC_TIMELINE, except that the filter will have its
> > + * inpad->filter_frame() callback(s) naturally called (and will use
> > + * AVFilterContext->is_disabled internally).
> 
> "naturally" ... "internally" ... "generic" are pretty vague terms and
> the result is confusing. Can you specify the meaning of "generic" and
> "internal" in a more precise way (and possibly to find some less vague
> terms may help).

This is my attempt. I just replaced "generic" with "default" since
default_filter_frame() is used, "internal" may be replaced by "auto"
or "automatic" but I'm not really satisfied with that as well, since
the user may guess that this is "automatically" executed by the
framework (rather than by the filter code).

Other alternatives to "generic":
external framework default

Other alternatives to "internal":
callback


/**
 * Some filters support a generic "enable" expression option that can be used
 * to enable or disable a filter in the timeline. Filters supporting this
 * option have this flag set. When the enable expression is false, the
 * default no-op filter_frame() function is called in place of the
 * filter_frame() callback defined on each input pad, thus the frame
 * is passed unchanged to the next filters.
 */
#define AVFILTER_FLAG_SUPPORT_TIMELINE_DEFAULT   (1 << 16)

/**
 * Same as AVFILTER_FLAG_SUPPORT_TIMELINE_DEFAULT, except that the filter will have its
 * filter_frame() callback(s) called as usual even when the enable
 * expression is false. The filter will disable filtering
 * within the filter_frame() callback(s) itself, for example executing
 * code depending on the AVFilterContext->is_disabled value.
 */
#define AVFILTER_FLAG_SUPPORT_TIMELINE_INTERNAL     (1 << 17)

Note that at this point I'm not against keeping the original terms
"generic" and "internal", but even in this case I suggest to specify
the "variant" after "timeline", as in
AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC.
-- 
FFmpeg = Forgiving & Fiendish Muttering Power Elected Governor


More information about the ffmpeg-devel mailing list