[FFmpeg-devel] [PATCH 2/4] lavfi: timeline support.
Clément Bœsch
ubitux at gmail.com
Tue Apr 16 03:31:42 CEST 2013
On Mon, Apr 15, 2013 at 11:33:13PM +0200, Stefano Sabatini wrote:
[...]
> > diff --git a/cmdutils.c b/cmdutils.c
> > index 4b625fd..c5aa000 100644
> > --- a/cmdutils.c
> > +++ b/cmdutils.c
> > @@ -1677,6 +1677,8 @@ static void show_help_filter(const char *name)
> > if (f->priv_class)
> > show_help_children(f->priv_class, AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_FILTERING_PARAM |
> > AV_OPT_FLAG_AUDIO_PARAM);
> > + if (f->flags & AVFILTER_FLAG_SUPPORT_TIMELINE)
> > + printf("This filter has support for timeline through the 'enable' option.\n");
>
> I'd rather prefer a flags line like for codecs (could be useful also
> for other flags like dynamic outputs).
>
Right now the two other flags are used to print a meaningful output, so
there is actually only this flag to print. Until more flags are added, I'd
prefer to keep it that way.
> > #else
> > av_log(NULL, AV_LOG_ERROR, "Build without libavfilter; "
> > "can not to satisfy request\n");
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 23d777d..d73d1ba 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -267,6 +267,33 @@ See the ``Quoting and escaping'' section in the ffmpeg-utils manual
> > for more information about the escaping and quoting rules adopted by
> > FFmpeg.
> >
> > + at chapter Timeline editing
> > +
> > +Some filters support a generic @option{enable} option. For the filters
> > +supporting timeline editing, this option can be set to an expression which is
> > +evaluated before sending a frame to the filter. If the evaluation is different
> > +from 0, the filter will be enabled, otherwise the frame will be send to the
>
> sent
>
Fixed.
> also "the frame will be sent ro the next..." looks confusing, since it
> implies that the frame is not sent otherwise. Maybe you want to say
> "will be sent unchanged".
>
Added "unchanged", thanks.
> > +next filter in the filtergraph.
> > +
> > +The expression accepts the following values:
> > + at table @samp
> > + at item t
> > +timestamp expressed in seconds, NAN if the input timestamp is unknown
> > +
> > + at item n
> > +sequential number of the input frame, starting from 0
> > + at end table
> > +
> > +Like any other filtering option, the @option{enable} option follows the same
> > +rules.
> > +
> > +For example, to enable a denoiser filter (@ref{hqdn3d}) from 10 seconds to 3
> > +minutes, and a @ref{curves} filter starting at 3 seconds:
>
> > + at example
> > +-vf 'hqdn3d = enable=between(t\,10\,3*60),
> > + curves = enable=gte(t\,3) : preset=cross_process'
> > + at end example
>
> Nit: please avoid ff* tools syntax in generic docs.
>
Changed locally to:
@example
hqdn3d = enable='between(t,10,3*60)',
curves = enable='gte(t,3)' : preset=cross_process
@end example
[...]
> > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > index 43340d1..8a5292d 100644
> > --- a/libavfilter/avfilter.c
> > +++ b/libavfilter/avfilter.c
> > @@ -23,6 +23,7 @@
> > #include "libavutil/avstring.h"
> > #include "libavutil/channel_layout.h"
> > #include "libavutil/common.h"
> > +#include "libavutil/eval.h"
> > #include "libavutil/imgutils.h"
> > #include "libavutil/opt.h"
> > #include "libavutil/pixdesc.h"
> > @@ -483,12 +484,20 @@ static const AVClass *filter_child_class_next(const AVClass *prev)
> > return NULL;
> > }
> >
> > +#define OFFSET(x) offsetof(AVFilterContext, x)
> > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM
> > +static const AVOption filters_common_options[] = {
> > + { "enable", "set enable expression", OFFSET(enable_str), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > + { NULL }
> > +};
>
> Note: is this shown in regular filter output help?
>
No, not yet. I'd better wait for more options: printing all the time that
enable option when it is available for a limited range of filters sound a
bit clumsy. The help mention that option so the user can look it up to see
how special it is.
Just like the flags, it will make sense to print them when we have more of
them, but currently that's not appropriate IMHO.
[...]
> > av_log(ctx, AV_LOG_DEBUG, "Setting '%s' to value '%s'\n", key, value);
> > +
> > + if (av_opt_find(ctx, key, NULL, 0, 0)) {
> > + ret = av_opt_set(ctx, key, value, 0);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > av_dict_set(options, key, value, 0);
> > if ((ret = av_opt_set(ctx->priv, key, value, 0)) < 0) {
> > if (!av_opt_find(ctx->priv, key, NULL, 0, AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ)) {
> > @@ -675,11 +699,27 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
> > return ret;
> > }
> > }
> > + }
>
> Can't you just set the option in the parent, and automatically look in
> the children contexts?
>
I wasn't able to do that yet, but I'll look it up.
> >
> > av_free(value);
> > av_free(parsed_key);
> > count++;
> > }
> > +
> > + if (ctx->enable_str) {
> > + if (!(ctx->filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE)) {
> > + av_log(ctx, AV_LOG_ERROR, "Timeline ('enable' option) not supported "
> > + "with filter '%s'\n", ctx->filter->name);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + ctx->var_values = av_calloc(VAR_VARS_NB, sizeof(*ctx->var_values));
> > + if (!ctx->var_values)
> > + return AVERROR(ENOMEM);
> > + ret = av_expr_parse((AVExpr**)&ctx->enable, ctx->enable_str, var_names,
> > + NULL, NULL, NULL, NULL, 0, ctx->priv);
> > + if (ret < 0)
> > + return ret;
>
> make sure this returns a meaningful error message (e.g. it should be
> clear that the parsing error occurred in the enable option)
>
[edgedetect @ 0x7f7524004180] [Eval @ 0x7f752bffe860] Missing ')' or too many args in 'gt(n,3'
Error initializing filter 'edgedetect' with args 'enable=gt(n,3'
> > + }
> > return count;
> > }
> >
> > @@ -852,6 +892,7 @@ static int default_filter_frame(AVFilterLink *link, AVFrame *frame)
> > static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
> > {
> > int (*filter_frame)(AVFilterLink *, AVFrame *);
> > + AVFilterContext *dstctx = link->dst;
> > AVFilterPad *dst = link->dstpad;
> > AVFrame *out;
> > int ret;
> > @@ -914,6 +955,12 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
> > }
> >
> > pts = out->pts;
> > + if (dstctx->enable_str) {
> > + dstctx->var_values[VAR_N] = link->frame_count;
> > + dstctx->var_values[VAR_T] = pts == AV_NOPTS_VALUE ? NAN : pts * av_q2d(link->time_base);
>
> > + if (!((int)av_expr_eval(dstctx->enable, dstctx->var_values, NULL)))
>
> uhm no you're doing my same mistake in select(), double->int is lossy
>
cast removed.
> > + filter_frame = default_filter_frame;
>
> > + }
> > ret = filter_frame(link, out);
> > link->frame_count++;
> > link->frame_requested = 0;
> > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > index 38bc5ee..a5518f7 100644
> > --- a/libavfilter/avfilter.h
> > +++ b/libavfilter/avfilter.h
> > @@ -428,6 +428,12 @@ enum AVMediaType avfilter_pad_get_type(const AVFilterPad *pads, int pad_idx);
> > * the options supplied to it.
> > */
> > #define AVFILTER_FLAG_DYNAMIC_OUTPUTS (1 << 1)
> > +/**
>
> > + * Some filter supports 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.
>
> this flag set?
>
Added. Also moved the trailing 's' of "supports" to the previous word.
> > + */
> > +#define AVFILTER_FLAG_SUPPORT_TIMELINE (1 << 16)
> >
> > /**
> > * Filter definition. This defines the pads a filter contains, and all the
> > @@ -522,7 +528,7 @@ typedef struct AVFilter {
> >
> > /** An instance of a filter */
> > struct AVFilterContext {
> > - const AVClass *av_class; ///< needed for av_log()
> > + const AVClass *av_class; ///< needed for av_log() and filters common options
> >
> > const AVFilter *filter; ///< the AVFilter of which this is an instance
> >
> > @@ -547,6 +553,10 @@ struct AVFilterContext {
> > struct AVFilterGraph *graph; ///< filtergraph this filter belongs to
> >
> > struct AVFilterCommand *command_queue;
> > +
>
> > + char *enable_str;
> > + void *enable;
>
> Nit+: doxy would be nice
>
Added.
[...]
> Should be good otherwise if reasonably tested.
Thanks, I will likely push the patches together though.
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130416/8a12e229/attachment.asc>
More information about the ffmpeg-devel
mailing list