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

Moritz Barsnick barsnick at gmx.net
Thu Dec 27 17:22:40 EET 2018


On Thu, Dec 27, 2018 at 13:00:29 +0100, Uwe Freese wrote:

These were just some remarks from me, still pending other, better
reviews.

>  > The "10" should be a #define, [...]
> 
> I have now added as error handling:
> 
> av_log(inlink->src, AV_LOG_ERROR, "More planes in frame than expected.\n");
> return AVERROR(ENOMEM);
> 
> Is this ok, or how should this be implemented instead?

I'll leave it up to others to tell you how many planes there actually
are.

> > Considering you authored it in 2018, this is forward-looking. ;-)
> 
> I thought it would take some days to review and when the code is 
> integrated finally, it is already 2019.
> 
> Should I write 2018, 2019 instead (assuming that it won't be integrated 
> before next week)?

I was basically kidding. But if it goes through this year, it's wrong.
Actually, the year of actual authorship is relevant AFAIK. So if you
have reused a lot of your original code, you could write "2001, 2018".
It probably doesn't matter.

> The original code had no spaces around operators in this case. To be 
> clear: Spaces are wanted here, right? So it should be "x = logo_x1 + 1" 
> etc. right?

It was unclear, due to reindentation (and probably moving code blocks
around) that this was original code. If you retain it functionally
(even when reindenting), please don't fix its whitespace. Only make
sure your new code adheres to the correct style.

> > Could power also be a double instead of an int? Would specifying a
> > power of e.g. 2.5 make sense?
> 
> This is a good point. It was an int value in VirtualDub's delogo and in 
> ffdshow, I think mostly because this int value was mapped to a slider in 
> the GUI...
> 
> There is the multiplier of 0.2 so the differences between the parameters 
> that are possible to set are small enough. Power of 2,5 is not needed 
> when the 0,2 factor is used.
> 
> But the question is, if a double parameter would be preferred instead of 
> an int (which is multiplied by 0,2)? The parameter to set by the user 
> would be e.g. "3" or "2.2" instead of "15" or "11".
> 
> I personally would prefer int parameters a little, but to explain the 
> functioning of the calculation to the user, a double might be better 
> ("At value 3, the weight for the consideration of a border pixel is 
> distance ^ 3.").
> 
> So shall I change this and use a double parameter?

This is basically up to you to decide. Is a "floating" power target
useful or not? If it's good enough in (e.g. 15) integral steps, do
leave it that way. It's really up to you.

> >> +    {"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).
> 
> I wasn't sure if one can rely on the fact that MODE_UGLARM is mapped to 
> 1 and is the max value, because it's not specified in the enum declaration.
> 
> But it seems we can be sure: 
> https://stackoverflow.com/questions/42128376/what-is-the-rule-for-assignment-of-the-integer-value-of-enum
> 
> So I changed it.

That's why I mentioned the MODE_NB style. I just checked an arbitrary
other filter and found libavfilter/f_metadata.c:

enum MetadataMode {
    METADATA_SELECT,
    METADATA_ADD,
    METADATA_MODIFY,
    METADATA_DELETE,
    METADATA_PRINT,
    METADATA_NB
};

The final _NB leaves room for adding other enums, and the valid range
always remains 0 .. METADATA_NB-1. That's nice style for anything which
potentially expands, but it's not mandatory.

> Also let me know if there's something else to change.

Yes, please get some more reviews on this.

> Subject: [PATCH] Add new delogo interpolation mode uglarm.

Commit message style: "avfilter/vf_delogo: add uglarm interpolation mode"

> + at item mode
> +Specify the interpolation mode used to remove the logo. 'xy' uses
> +only the border pixels in straight x and y direction to replace any
> +logo pixel. It tends to flicker and to show horizontal and vertical
> +lines. 'uglarm' considers the whole border for every logo pixel to
> +replace. It uses the power of the distance to any border pixel as
> +weight to which amount it's taken into account. This results in a
> +more blurred area, which tends to be less distracting. The default
> +value is 'xy'.

Use @var{xy} and @var{uglarm}.
"it's" -> "it is" preferred.

You could also use the format:
@item mode
Intro text.

It accepts the following values:
@table @option
@item xy
This mode uses...
This mode is the default.
@item uglarm
This mode considers...
@end table

> + at item power
> +Specify the power (factor) used to calculate the weight out of the
> +distance in 'uglarm' mode (weight = distance ^ 0.2 * power). The
> +value 0 results in a logo area which has the same average color
> +everywhere. The higher the value, the more relevant a near border
> +pixel will get, meaning that the borders of the logo area are more
> +similar to the surrounding pixels. The default value 15 results in
> +power 3 (= 0.2 * 15).

Otherwise okay.

> +    if (s->mode == MODE_UGLARM)
> +    {

if ( ... ) {

No further comments from me. The patch applies, the filter works, and
it's only slightly slower than the xy mode. I couldn't find any good
material to compare the result thouh. Good work anyway!

Cheers,
Moritz


More information about the ffmpeg-devel mailing list