[FFmpeg-devel] [PATCH 2/3] lavfi: timeline support.

Clément Bœsch ubitux at gmail.com
Sat Apr 20 15:21:56 CEST 2013


On Sat, Apr 20, 2013 at 11:40:39AM +0200, Stefano Sabatini wrote:
> Nit+++++: lavfi: add timeline support
> 

Changed.

[...]
> > + 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 sent unchanged
> 
> nit: different from zero -> non-zero?
> 
> seems less clumsy
> 

Changed

> > +to the 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
> > +hqdn3d = enable='between(t,10,3*60)',
> > +curves = enable='gte(t,3)' : preset=cross_process
> > + at end example
> 
> I wonder how we could allow to read intervals from a file.
> 

?

> Second note: how do you think it would be possible to implement a
> generic process_command() callback?
> 

Added in the following patch; see my reply to your overlay review.

[...]
> >  static const AVClass avfilter_class = {
> >      .class_name = "AVFilter",
> >      .item_name  = default_filter_name,
> >      .version    = LIBAVUTIL_VERSION_INT,
> >      .category   = AV_CLASS_CATEGORY_FILTER,
> >      .child_next = filter_child_next,
> > +    .option     = filters_common_options,
> >      .child_class_next = filter_child_class_next,
> >  };
> 
> I'm happy we have AVFILTER_DEFINE_CLASS.
> 

How would that help here?

[...]
> > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > index 38bc5ee..7df42f7 100644
> > --- a/libavfilter/avfilter.h
> > +++ b/libavfilter/avfilter.h
> > @@ -385,6 +385,19 @@ struct AVFilterPad {
> >      int needs_fifo;
> >  
> >      int needs_writable;
> > +
> > +    /**
> > +     * Passthrough filtering callback.
> > +     *
> 
> > +     * If a filter support timeline editing (AVFILTER_FLAG_SUPPORT_TIMELINE)
> 
> supportS
> 

Changed.

> (in case the AVFILTER_FLAG_SUPPORT_TIMELINE is enabled)
> 

Forgot to change this in the patch I re-sent. Fixed localley.

> > +     * 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 is obviously not supposed to do
> 
> s/obviously//
> 
> also maybe state it more strongly: The filter must not do any change...
> 

Reworded with your suggestion.

> > +     * any change to the frame in this callback.
> > +     *
> 
> > +     * Input pads only.
> 
> Flying saucepans only.
> 
> Context and verb missing.
> 

I merely copied the same sentence as the other doxy.

> > +     */
> > +    int (*passthrough_filter_frame)(AVFilterLink *link, AVFrame *frame);
> >  };
> >  #endif
> >  
> > @@ -428,6 +441,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)
> > +/**
> 
> nit++: missing separating empty line?
> 

There wasn't between the two others so I didn't add one here.

> > + * 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.
> > + */
> > +#define AVFILTER_FLAG_SUPPORT_TIMELINE      (1 << 16)
> [...]
> 
> LGTM otherwise, but give some time to Nicolas to reply.
> 

Will wait a few days, thanks. (See overlay answer for the update, sorry).

-- 
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/20130420/cb501774/attachment.asc>


More information about the ffmpeg-devel mailing list