[FFmpeg-devel] [PATCH] rotate filter

[Stefano Sabatini]

On date Wednesday 2013-06-12 09:56:36 +0200, Clément Bœsch encoded:
> On Tue, Jun 11, 2013 at 03:12:31PM +0200, Stefano Sabatini wrote:
> [..]
> > From e92156a05861c55699e4460fc1b9df1204dc7171 Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefasab at gmail.com>
> > Date: Tue, 11 Jun 2013 10:31:59 +0200
> > Subject: [PATCH] lavfi: add rotate filter
> > 
> > Based on the libavfilter SOC filter by Vitor Sessak, with the following additions:
> > * integer arithmetic
> > * bilinear interpolation
> > * RGB path
> > * configurable parametric angle, output width and height
> > 
> > Address trac issue #1500.
> > ---
> >  doc/filters.texi         |  111 +++++++++++
> >  libavfilter/Makefile     |    1 +
> >  libavfilter/allfilters.c |    1 +
> >  libavfilter/vf_rotate.c  |  456 ++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 569 insertions(+)
> >  create mode 100644 libavfilter/vf_rotate.c
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 4cb6710..7e8fff9 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -5741,6 +5741,117 @@ pp=hb|y/vb|a
> >  @end example
> >  @end itemize
> >  
> > + at section rotate
> > +
> 
> > +Rotate video by a chosen amount in degrees. By default, 45 degrees.
> 
> chosen → arbitrary?
> 
> "chosen" sounds like a selection between a number of choice; at first I
> thought it was a transpose like filter.
> 
> > +
> > +The filter accepts the following options:
> > +
> > +A description of the optional parameters follows.
> > + at table @option
> > + at item angle, a
> > +Set an expression for the angle by which to rotate the input video
> > +clockwise, expressed in radians degrees.
> 
> radians or degrees?

Fixed here and everywhere.

[...]
> > + at item
> > +Rotate the input by PI/6 degrees counter-clockwise:
> > + at example
> > +rotate=-PI/6
> > + at end example
> > +
> > + at item
> 
> > +Apply a constant rotation with period $var{T}, starting from an angle of PI/3:
> 
> $ → @
> 
> Make sure the rest of the HTML output is OK.
> 
[...]
> > + at item
> > +Rotate the video, output size is choosen so that the whole rotating
> > +input video is always completely contained in the output:
> > + at example
> 
> > +rotate=2*PI*t:ow=sqrt(iw*iw+ih*ih):oh=ow
> 
> hypot()

Nice find.

[...]
> > +static int config_props(AVFilterLink *outlink)
> > +{
> > +    AVFilterContext *ctx = outlink->src;
> > +    RotContext *rot = ctx->priv;
> > +    AVFilterLink *inlink = ctx->inputs[0];
> > +    const AVPixFmtDescriptor *pixdesc = av_pix_fmt_desc_get(inlink->format);
> > +    uint8_t rgba_color[4];
> > +    int i, is_packed_rgba, ret;
> > +    double res;
> > +    char *expr;
> > +
> > +    rot->hsub = pixdesc->log2_chroma_w;
> > +    rot->vsub = pixdesc->log2_chroma_h;
> > +
> > +    rot->var_values[VAR_IN_W]  = rot->var_values[VAR_IW] = inlink->w;
> > +    rot->var_values[VAR_IN_H]  = rot->var_values[VAR_IH] = inlink->h;
> > +    rot->var_values[VAR_HSUB]  = 1<<rot->hsub;
> > +    rot->var_values[VAR_VSUB]  = 1<<rot->vsub;
> > +    rot->var_values[VAR_N]     = 0;
> > +    rot->var_values[VAR_T]     = 0;
> > +    rot->var_values[VAR_A]     = NAN;
> > +    rot->var_values[VAR_OUT_W] = rot->var_values[VAR_OW] = NAN;
> > +    rot->var_values[VAR_OUT_H] = rot->var_values[VAR_OH] = NAN;
> > +
> > +    if ((ret = av_expr_parse(&rot->angle_expr, rot->angle_expr_str, var_names,
> 
> (expr = rot->angle_expr_str) and goto eval_fail?

Changed.

> > +                             func1_names, func1, NULL, NULL, 0, ctx)) < 0) {
> > +        av_log(ctx, AV_LOG_ERROR,
> > +               "Error occurred parsing expression '%s'\n", rot->angle_expr_str);
> > +        return ret;
> > +    }
> > +
> > +    res = av_expr_eval(rot->angle_expr, rot->var_values, NULL);
> > +    if (isnan(res)) {
> > +        av_log(ctx, AV_LOG_ERROR,
> 
> > +               "Invalid angle expression '%s', evaluates to nan.\n",
> 
> nit: NaN
> 
[...]
> > +    /* compute number of planes */
> > +    rot->nb_planes = 0;
> > +    for (i = 0; i < 4; i++) {
> > +        const AVComponentDescriptor *comp = &(pixdesc->comp[i]);
> 
> nit: pointless ( )
> 
> > +        rot->nb_planes = FFMAX(rot->nb_planes, comp->plane);
> > +    }
> > +    rot->nb_planes++;

Now I'm using av_pix_fmt_count_planes().

> > +
> 
> > +    rot->outw &= ~((1 << rot->hsub) - 1);
> > +    rot->outh &= ~((1 << rot->vsub) - 1);
> 
> Can't you use on of the available macro for this?

Looks like they are not required after all...

[...]
> > +AVFilter avfilter_vf_rotate = {
> > +    .name        = "rotate",
> > +    .description = NULL_IF_CONFIG_SMALL("Rotate the input image."),
> > +    .priv_size   = sizeof(RotContext),
> > +    .init        = init,
> > +    .uninit      = uninit,
> 
> > +    .query_formats = query_formats,
> 
> nit++: use this one for align basis.

Done. 
 
> > +    .inputs      = rotate_inputs,
> > +    .outputs     = rotate_outputs,
> > +    .priv_class  = &rotate_class,
> > +};

I also added timeline support, which works for free out of the box.
 
> I didn't find anything obviously wrong, but I didn't look at the math
> involved. Patch LGTM.

I'll test it more, I'll add a test, and push it if I don't see more
comments.
-- 
FFmpeg = Free and Forgiving Murdering Portentous Elastic Geek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavfi-add-rotate-filter.patch
Type: text/x-diff
Size: 21899 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130612/d86955a5/attachment.bin>