[FFmpeg-soc] [RFC] Apply function to pixel filter: vf_applyfn

[Stefano Sabatini]

On date Monday 2009-05-25 15:47:59 +0200, zeger at customsoft.nl encoded:
> 
> Hi, 
> 
> ----- "Stefano Sabatini" <stefano.sabatini-lala at poste.it> wrote: 
> > I believe pow *is* supported out of the box by the eval code. 
> 
> It is not enabled in the parse_primary(Parser *p) function.
> 
> > Docs and example go to doc/vfilters.texi. 
> 
> Removed them.

Uh, what about to move them to doc/vfilters.texi?
 
> > enum VarName { 
> > ... 
> > VARS_NB 
> > }; 
> > 
> > is better. 
> 
> Done.
> 
> > 
> > And a PTS var may be useful too. 
> > 
> 
> Working on that.
> 
> > > +static const char *applyfn_symbols[POV_NULL+1]={ 
> >                                               ^^ 
> > Why +1? 
> 
> I copied the code from vf_setpts, didn't gave it second thought. 
> 
> > > +        ApplyfnContext *applyfn = ctx->priv;
> > > +        char Y_expr[256], U_expr[256], V_expr[256], A_expr[256];
> > > +       const char *error;
> > 
> > Weird indent. 
> > 
> 
> Fixed.
> 
> > > + sscanf(args, "%255[^:]:%255[^:]:%255[^:]:%255[^:]", Y_expr, U_expr, V_expr, A_expr) 
> > 
> > I believe here it would be nice to use av_set_options_string, check 
> > vf_pad.c in the archive for an usage example. 
> 
> Done.
> 
> > > +        applyfn->value_V = ff_parse(V_expr, applyfn_symbols,
> > > +                                NULL, NULL, NULL, NULL, &error);
> > > +        if (!applyfn->value_V )
> > > +            av_log(ctx, AV_LOG_ERROR,
> > > +                   "init() cannot parse V expression due to %s\n", error);
> > > +
> > > +        applyfn->value_A = ff_parse(A_expr, applyfn_symbols,
> > > +                                NULL, NULL, NULL, NULL, &error);
> > > +        if (!applyfn->value_A )
> > > +            av_log(ctx, AV_LOG_ERROR,
> > > +                   "init() cannot parse A expression due to %s\n", error);
> > This can be factorized (check again vf_pad.c to see how), maybe even a 
> > macro may be used. 
> 
> I created a macro for it, I'm not sure if it is okay according to 
> FFmpeg coding standards.
> 
> > > +static void draw_slice(AVFilterLink *link, int y, int h)
> > > +{
> > > +    ApplyfnContext *applyfn = link->dst->priv;
> > > +    AVFilterPicRef *in  = link->cur_pic;
> > > +    AVFilterPicRef *out = link->dst->outputs[0]->outpic;
> > > +    uint8_t *inrow_0, *outrow_0, *alpharow, *outrow_1, *outrow_2, *inrow_1, *inrow_2;
> > > +    int i, j, i_sub, j_sub, outrow_1_off, outrow_2_off, inrow_1_off, inrow_2_off;
> > > +
> > > +    applyfn->values[POV_N] += 1.0;
> > 
> > Ouch, this is wrong, as draw_slice() is called for every slice(), and 
> > you don't know how many slices there are. This update should be done 
> > in start_frame() / end_frame() (same for W and H, since they're not 
> > supposed to change between a slice and another one). 
> > 
> 
> Moved the code to start_frame. 
> 
> For some functions it would be much easier to normalize the YUV values
> to [0.0..1.0][-0.5..0.5][-0.5..0.5] beforehand, would it be an addition 
> if this were to be an option? 

Yes, I believe so. Maybe simply adding some vars for the normalized
values will be fine.

> Thanks for the feedback!
> 
> Zeger Knops

> Index: vf_applyfn.c
[...]
> +static void draw_slice(AVFilterLink *link, int y, int h)
> +{
> +    ApplyfnContext *applyfn = link->dst->priv;
> +    AVFilterPicRef *in  = link->cur_pic;
> +    AVFilterPicRef *out = link->dst->outputs[0]->outpic;
> +    uint8_t *inrow_0, *outrow_0, *alpharow, *outrow_1, *outrow_2, *inrow_1, *inrow_2;
> +    int i, j, i_sub, j_sub, outrow_1_off, outrow_2_off, inrow_1_off, inrow_2_off;
> +
> +    inrow_0  = in-> data[0] + y * in-> linesize[0];
> +    outrow_0 = out->data[0] + y * out->linesize[0];
> +
> +    outrow_1_off = (y >> applyfn->vsub)  * out->linesize[1];
> +    outrow_2_off = (h >> applyfn->vsub ) * out->linesize[2] * ( link->w >> applyfn->hsub ) * (y >> applyfn->vsub);
> +    inrow_1_off  = (y >> applyfn->vsub)  * in-> linesize[1];
> +    inrow_2_off  = (h >> applyfn->vsub ) * in-> linesize[2] * ( link->w >> applyfn->hsub ) * (y >> applyfn->vsub);
> +
> +    outrow_1 = outrow_1_off + out->data[1];
> +    outrow_2 = outrow_2_off + out->data[2];
> +    inrow_1  = outrow_1_off + in-> data[1];
> +    inrow_2  = outrow_2_off + in-> data[2];

Some nits:

> +    if ( out->data[3] )

I believe (out->data[3]) style is preferred.

> +        alpharow = out->data[3] + y * out->linesize[3];
> +
> +    for(i = 0; i < h; i ++) {
> +        applyfn->var_values[y] = i;
> +        i_sub = i >> applyfn->hsub;
> +        for(j = 0; j < link->w; j ++)
> +        {
> +           j_sub = j >> applyfn->hsub;

Not K&R style (for brace not on the same line as the for), and weird
indent.

> +            applyfn->var_values[x] = j;
> +
> +            applyfn->var_values[Y] = inrow_0[j];
> +            applyfn->var_values[U] = inrow_1[j_sub];
> +            applyfn->var_values[V] = inrow_2[j_sub];
> +
> +            applyfn->var_values[A] = out->data[3] ? alpharow[j] : 0;
> +
> +            outrow_0[j]     = ff_parse_eval(applyfn->Y_evalexpr, applyfn->var_values, applyfn);
> +            outrow_1[j_sub] = ff_parse_eval(applyfn->U_evalexpr, applyfn->var_values, applyfn);
> +            outrow_2[j_sub] = ff_parse_eval(applyfn->V_evalexpr, applyfn->var_values, applyfn);
> +
> +            if ( out->data[3] )
                  ^^^^^^^^^^^^^^^^
> +                alpharow[j] = ff_parse_eval(applyfn->A_evalexpr, applyfn->var_values, applyfn);
> +
> +        }
> +        inrow_0  += in-> linesize[0];
> +        outrow_0 += out->linesize[0];
> +
> +        if ( out->data[3] )
              ^^^^^^^^^^^^^^^^

> +            alpharow += out->linesize[3];
> +
> +        inrow_1  = inrow_1_off  + in-> data[1] + i_sub * in-> linesize[1];
> +        inrow_2  = inrow_2_off  + in-> data[2] + i_sub * in-> linesize[2];
> +        outrow_1 = outrow_1_off + out->data[1] + i_sub * out->linesize[1];
> +        outrow_2 = outrow_2_off + out->data[2] + i_sub * out->linesize[2];
> +    }
> +
> +    avfilter_draw_slice(link->dst->outputs[0], y, h);
> +}
> +
> +AVFilter avfilter_vf_applyfn =
> +{
> +    .name      = "applyfn",
> +    .init      = init,

Rethinking at it, I wrote a very similar filter named vf_eval.c but
which worked in the RGB colorspace.

Maybe it would be less confusing for the users to give to this filters
some related names, for example:
rgbeval and yuveval,
or
yuvapplyfn and rgbapplyfn.

If you don't mind consider to change accordingly the name of the
filter (but await for others devs opinion, Vitor?).

Regards.