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

Uwe Freese uwe.freese at gmx.de
Thu Dec 27 14:00:29 EET 2018


Hello,

thanks for the comments. Most points were clear and I've changed the 
code accordingly (see attached new patch).

Here are the remaining questions / points to discuss:

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

I've added the first version in the patch attached. Maybe some of the 
sentences have to be improved - I'm not an english native speaker.

 > 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?

>> + * Copyright (c) 2019 Uwe Freese <mail at uwe-freese.de>
> 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)?

>
>> +            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.)

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?

>
>> +    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?

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?

>
>> +    {"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.


It would be nice to get some infos and opinions about these questions. 
I'll change the code accordingly afterwards.
Also let me know if there's something else to change.

Regards,
Uwe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-new-delogo-interpolation-mode-uglarm.patch
Type: text/x-patch
Size: 13757 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181227/674c02d9/attachment.bin>


More information about the ffmpeg-devel mailing list