[FFmpeg-devel] [PATCH] lavfi/WIP: vignette filter.

[Clément Bœsch]

On Sun, May 12, 2013 at 03:40:25PM +0200, Stefano Sabatini wrote:
[...]
> > + at table @option
> > + at item angle, a
> > +Set lens angle expression.
> 
> ... as a number of radians, or people will invariably try to use silly
> 1/360 degrees.
> 

Added.

> > +
> > +The value is clipped in the @code{[0,PI/2]} range.
> > +
> > +Default value: "PI/5"
> 
> An explanation of the meaning of this angle would be welcome.
> 

I'm not sure what I could add, it's the lens angle...

[...]
> > + at item t
> > +the PTS (Presentation TimeStamp) of the filtered video frame,
> > +expressed in seconds, NAN if undefined
> 
> technically a time is not a PTS, more accurate:
> the PTS (Presentation TimeStamp) *time*
> 

Added, but note that it was copy/pasted verbatim from another filter
(overlay IIRC).

> > +
> > + at item tb
> > +time base of the input video
> > + at end table
> > +
> > +
> > + at subsection Examples
> > +
> > + at itemize
> 
> > + at item
> > +Simple strong vignetting effect:
> 
> Apply simple ...
> 

Changed.

> > + at example
> > +vignette=PI/4
> > + at end example
> > +
> > + at item
> > +Flickering vignetting:
> 
> Apply flickering ...
> 

Formulated differently.

[...]
> > +static av_cold int init(AVFilterContext *ctx)
> > +{
> > +    int ret;
> > +    VignetteContext *s = ctx->priv;
> > +
> > +    if ((ret = av_expr_parse(&s->a_pexpr,  s->a_expr,  var_names, NULL, NULL, NULL, NULL, 0, ctx)) < 0 ||
> > +        (ret = av_expr_parse(&s->x0_pexpr, s->x0_expr, var_names, NULL, NULL, NULL, NULL, 0, ctx)) < 0 ||
> > +        (ret = av_expr_parse(&s->y0_pexpr, s->y0_expr, var_names, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
> 
> You could create a macro to specify the name of expression which
> failed.
> 

Done.

[...]
> > +#define TS2D(ts)     ((ts) == AV_NOPTS_VALUE ? NAN : (double)(ts))
> > +#define TS2T(ts, tb) ((ts) == AV_NOPTS_VALUE ? NAN : (double)(ts) * av_q2d(tb))
> 
> we may move these to a common header...
> 

Feel free to add it to libavutil/timestamp.h or wherever

[...]
> > +static int config_props(AVFilterLink *inlink)
> 
> nit: move this before filter_frame()
> 

I want to keep filter_frame and the 3 functions above together. But I
can't move config_props above those 4 functions, so I keep it here.

[...]

See the new version of the patch is Michael's reply. Thanks for the
review.

-- 
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/20130529/de5daf27/attachment.asc>