[FFmpeg-devel] [PATCH] delogo filter: new "uglarm" interpolation mode added

[Moritz Barsnick]

On Wed, Dec 26, 2018 at 15:23:58 +0100, Uwe Freese wrote:

Just some initial remarks (no full review):

> Can someone please review and add the code? Who would be primarily 
> responsible to add this to libavfilter?

> ./ffmpeg -i "yourtestvideo.mkv" -map 0:v:0 -vframes 100 -filter_complex 
> "crop=200:200:100:100,split=2[L][R];[L]delogo=x=50:y=50:w=100:h=100[L2];[R]delogo=x=50:y=50:w=100:h=100:mode=uglarm:power=15[R2];[L2][R2]hstack=inputs=2" 
> -c:v huffyuv delogotest.mkv && ffplay delogotest.mkv

ffmpeg can display directly to screen as well, BTW.

>    Note: I preferred using a string / enum parameter instead of a 
> boolean so the parameter could stay backwards compatible in case another 
> mode would be added some day.

Perfect.

>    Question: How many planes can there be? I used an array of pointers 
> to 10 possible lookup tables (one per plane).

The "10" should be a #define, so you only need to change it in one
place if you figure out that the correct number of planes is different.
;-) (For your own ease, but also for understanding where the "10" comes
from.)

> - I tried to use the same formatting etc. as in the original source 
> code. Is there anything I have to change?

You almost did. You already heard about indentation and especially the
added whitespace. You became inconsistent in some places, such as here:

>      /* Actual default value for band/t is 1, set in init */
> -    { "band", "set delogo area band size", OFFSET(band), AV_OPT_TYPE_INT, { .i64 =  0 },  0, INT_MAX, FLAGS },
> -    { "t",    "set delogo area band size", OFFSET(band), AV_OPT_TYPE_INT, { .i64 =  0 },  0, INT_MAX, FLAGS },
> +    { "band",   "set delogo area band size",                OFFSET(band),   AV_OPT_TYPE_INT, { .i64 =  0 },  0, INT_MAX, FLAGS },
> +    { "t",      "set delogo area band size",                OFFSET(band),   AV_OPT_TYPE_INT, { .i64 =  0 },  0, INT_MAX, FLAGS },
>  #endif
> -    { "show", "show delogo area",          OFFSET(show), AV_OPT_TYPE_BOOL,{ .i64 =  0 },  0, 1,       FLAGS },
> +    {"mode",    "set the interpolation mode",               OFFSET(mode), AV_OPT_TYPE_INT, { .i64 = MODE_XY},      0, 1, FLAGS, "mode"},
> +        {"xy",     "use pixels in straight x any y direction",  OFFSET(mode), AV_OPT_TYPE_CONST, { .i64 = MODE_XY},     0, 0, FLAGS, "mode"},
> +        {"uglarm", "UGLARM mode, use full border",              OFFSET(mode), AV_OPT_TYPE_CONST, { .i64 = MODE_UGLARM}, 0, 0, FLAGS, "mode"},
> +    { "power",  "power of UGLARM interpolation",            OFFSET(power),  AV_OPT_TYPE_INT, { .i64 = 15 },  0,      30, FLAGS },

Note how the existing original code had '{ "string",...', and you chose
to drop some whitespace. That's just cosmetic, but inconsistent. You
also have mismatching indentation.

> Of course, I would support adding documentation about the filter as 
> well. Just let me know what's needed.

You can start writing it now already, because it needs to go into
doc/filters.texi.

> + * Copyright (c) 2019 Uwe Freese <mail at uwe-freese.de>

Considering you authored it in 2018, this is forward-looking. ;-)

>      uint8_t *xdst, *xsrc;
> -
> +                
>      uint8_t *topleft, *botleft, *topright;

See, this, among others, is what we mean with whitespace. If you had
looked through your patch, you would have noticed this useless change.
(Some editors and some patch viewers point out this danglig
whitespace.)

> +            left_sample = topleft[src_linesize*(y-logo_y1)]   +
> +                        topleft[src_linesize*(y-logo_y1-1)] +
> +                        topleft[src_linesize*(y-logo_y1+1)];

The "topleft"s and the '+'s used to be column-aligned. You should
ensure that stays that way (in the indentation patch of course, if it's
indentation related).

> +            for (x = logo_x1+1,
> +                xdst = dst+logo_x1+1,
> +                xsrc = src+logo_x1+1; x < logo_x2; x++, xdst++, xsrc++) {

Spaces around operators: x = logo_x1 + 1
(Also everywhere else. Unless it's the original code, then leave it be.)

> + * @param *uglarmtable      Pointer to table containing weigths for each possible

weigths -> weights

> +static void calcUGLARMTables(double *uglarmtable, double *uglarmweightsum,
> +                             AVRational sar, int logo_w, int logo_h, int power)

No capital letters or CamelCase in function names.

> +    double e = 0.2 * power;

Could power also be a double instead of an int? Would specifying a
power of e.g. 2.5 make sense?

> +    /* uglarmtable will contain a weigth for each possible diagonal distance

weigth -> weight

> +                double d = pow(sqrt((double)(x * x * aspect * aspect + y * y)), e);

int * int * double * double + int * int
already results in a double, unless I am mistaken. No need to cast the
result.

> +    /* uglarmweithsum will conatain the sum of all weigths which is used when

uglarmweithsum -> uglarmweightsum
conatain -> contain
weigths -> weights

> +    {"mode",    "set the interpolation mode",               OFFSET(mode), AV_OPT_TYPE_INT, { .i64 = MODE_XY},      0, 1, FLAGS, "mode"},

min and max are MODE_XY and MODE_UGLARM (or MODE_NB-1, if you code it
that way to give room for more modes).

> +        {"xy",     "use pixels in straight x any y direction",  OFFSET(mode), AV_OPT_TYPE_CONST, { .i64 = MODE_XY},     0, 0, FLAGS, "mode"},

any -> and

> +        for (int plane = 0; plane < 10; plane++) {

As mentioned, 10 could be a #define.

> +            if (s->uglarmtable[plane])
> +                av_free(s->uglarmtable[plane]);
> +            if (s->uglarmweightsum[plane])
> +                av_free(s->uglarmweightsum[plane]);

Please read the av_free() documentation. No need for a NULL check.
https://www.ffmpeg.org/doxygen/4.1/group__lavu__mem__funcs.html#ga0c9096f498624c525aa2315b8a20c411

Moritz