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

Stefano Sabatini stefasab at gmail.com
Sun May 12 12:11:04 CEST 2013


On date Saturday 2013-05-11 22:48:02 +0200, Clément Bœsch encoded:
> On Sat, May 11, 2013 at 04:50:36PM +0200, Stefano Sabatini wrote:
> [...]
> > 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.
> 
> Perfectly fine, thank you. New patch attached. I also added an assert to
> make sure both default & internal are not set.
> 
> -- 
> Clément B.

> From 54c52c2ded283f30509c5da394f4de6d4b1f6b7d 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 small compat break, but the timeline was
> introduced very recently.
> 
> Doxy by Stefano Sabatini.
> ---
>  libavfilter/af_apad.c              |  3 +--
>  libavfilter/af_volume.c            |  2 +-
>  libavfilter/avfilter.c             |  9 ++++++---
>  libavfilter/avfilter.h             | 33 ++++++++++++++++++---------------
>  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 +-
>  23 files changed, 48 insertions(+), 60 deletions(-)

Reminder: bump minor, update APIchanges.

> 
> diff --git a/libavfilter/af_apad.c b/libavfilter/af_apad.c
> index 265b76a..710baaa 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_SUPPORT_TIMELINE_INTERNAL,
>  };
> diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c
> index b7aec8d..d9a5d4a 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_SUPPORT_TIMELINE_DEFAULT,
>  };
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 2b95b65..2f30ad9 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -453,6 +453,9 @@ int avfilter_register(AVFilter *filter)
>      AVFilter **f = &first_filter;
>      int i;
>  
> +    /* the filter must select internal or default */

Nit: ... or default exclusively

> +    av_assert0((filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE) != AVFILTER_FLAG_SUPPORT_TIMELINE);
> +
>      for(i=0; filter->inputs && filter->inputs[i].name; i++) {
>          const AVFilterPad *input = &filter->inputs[i];
>          av_assert0(     !input->filter_frame
> @@ -995,9 +998,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_SUPPORT_TIMELINE_DEFAULT))
> +            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..fe0c3e0 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
>  
> @@ -444,9 +431,25 @@ enum AVMediaType avfilter_pad_get_type(const AVFilterPad *pads, int pad_idx);
>  /**
>   * 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.
> + * 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.

Nit: weird justification.

> + */
> +#define AVFILTER_FLAG_SUPPORT_TIMELINE_INTERNAL (1 << 17)

Note:
AVFILTER_FLAG_SUPPORT_TIMELINE_DEFAULT

I realize the "default" is a bit confusing.

Alternatively:
AVFILTER_FLAG_SUPPORT_TIMELINE
AVFILTER_FLAG_SUPPORT_TIMELINE_INTERNAL

or you can use GENERIC instead like in the first patch. I leave this
up to you.

[...]

LGTM otherwise.
-- 
FFmpeg = Fascinating Friendly Monstrous Plastic Exploitable Gnome


More information about the ffmpeg-devel mailing list