[FFmpeg-devel] [PATCH] lavfi/crop: add support for x/y commands

[Michael Niedermayer]

On Sun, Jul 14, 2013 at 03:54:58PM +0200, Michael Niedermayer wrote:
> On Wed, Jul 10, 2013 at 05:51:08PM +0200, Stefano Sabatini wrote:
> > TODO: bump micro
> > ---
> >  doc/filters.texi      | 14 ++++++++++++++
> >  libavfilter/vf_crop.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 67 insertions(+)
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 92f8612..9e95f2c 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -2503,6 +2503,20 @@ is approximated to the nearest valid value.
> >  The expression for @var{x} may depend on @var{y}, and the expression
> >  for @var{y} may depend on @var{x}.
> >  
> > + at subsection Commands
> > +
> > +This filter supports the following commands:
> > + at table @option
> > + at item x
> > + at item y
> > + at item H
> > +Modify the x and/or the y position of the cropped area.
> > +The command accepts the same syntax of the corresponding option.
> > +
> > +If the specified expression is not valid, it is kept at its current
> > +value.
> > + at end table
> > +
> >  @subsection Examples
> >  
> >  @itemize
> > diff --git a/libavfilter/vf_crop.c b/libavfilter/vf_crop.c
> > index 832e6cf..4c63107 100644
> > --- a/libavfilter/vf_crop.c
> > +++ b/libavfilter/vf_crop.c
> > @@ -292,6 +292,58 @@ static int filter_frame(AVFilterLink *link, AVFrame *frame)
> >      return ff_filter_frame(link->dst->outputs[0], frame);
> >  }
> >  
> > +static int set_expr(AVExpr **pexpr_ptr, char **expr_ptr,
> > +                    const char *expr, const char *option, void *log_ctx)
> > +{
> > +    int ret;
> > +    AVExpr *new_pexpr;
> > +    char *new_expr;
> > +
> > +    new_expr = av_strdup(expr);
> > +    if (!new_expr)
> > +        return AVERROR(ENOMEM);
> > +    ret = av_expr_parse(&new_pexpr, expr, var_names,
> > +                        NULL, NULL, NULL, NULL, 0, log_ctx);
> > +    if (ret < 0) {
> > +        av_log(log_ctx, AV_LOG_ERROR,
> > +               "Error when evaluating the expression '%s' for %s\n",
> > +               expr, option);
> > +        av_free(new_expr);
> > +        return ret;
> > +    }
> > +
> > +    if (*pexpr_ptr)
> > +        av_expr_free(*pexpr_ptr);
> > +    *pexpr_ptr = new_pexpr;
> > +    av_freep(expr_ptr);
> > +    *expr_ptr = new_expr;
> > +
> > +    return 0;
> > +}
> > +
> > +static int process_command(AVFilterContext *ctx, const char *cmd, const char *args,
> > +                           char *res, int res_len, int flags)
> > +{
> > +    CropContext *crop = ctx->priv;
> > +
> > +#define SET_EXPR(expr, option)                                          \
> > +    do {                                                                \
> > +        int ret = set_expr(&crop->expr##_pexpr, &crop->expr##_expr,     \
> > +                           args, option, ctx);                          \
> > +        if (ret < 0)                                                    \
> > +            return ret;                                                 \
> > +    } while (0)
> > +
> > +    if (!strcmp(cmd, "x")) {
> > +        SET_EXPR(x, "x");
> > +    } else if (!strcmp(cmd, "y")) {
> > +        SET_EXPR(y, "y");
> > +    } else
> > +        return AVERROR(ENOSYS);
> 
> the macro obfuscation can be avoided and a simple function and
> pointers be used

I worded this unclear, what i meant was:

    if (!strcmp(cmd, "x")) {
        ret = set_expr(&crop->x_pexpr, &crop->x_expr, args, "x", ctx);
    } else if (!strcmp(cmd, "y")) {
        ret = set_expr(&crop->y_pexpr, &crop->y_expr, args, "y", ctx);
    } else
        return AVERROR(ENOSYS);

    if (ret < 0)
        return ret;


> 
> also
> the user might have new options like  "x=y,y=6" or "y=x,x=5" or
> "x=4,y=x"
> the order by which you parse this string and submit it affects the
> results.
> 
> And why dont you flag settable AVOptions and use generic code to set
> them through some command that is handled in the generic code ?
> 
> The expression parsing that already exists in filters could be
> reused instead of duplicating it for the duplicated command processig
> this also would avoid any order of evaluation issues as its the same
> code either way no matter what did set the parameters
> this also automatically avoids bugs with parameter validation and
> rounding related to chroma subsamping as either way the same code
> is used

I withdraw these comments, it wasnt my intent to filibuster the work.
if the design as in the patch works out simpler and better, all
fine with me, go ahead with it.


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130829/b0b4b1aa/attachment.asc>