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

[Uwe Freese]

Hello,

Here's a new version of the patch. Changes:


- My last version didn't compile because of moving code to config_input. 
Don't know why I didn't see this, sorry.
   I moved the code back to filter_frame because of two reasons. First, 
I need the "sar" to init my tables and it seems I need the "AVFrame *in" 
parameter for that. Secondly, I also need *desc, hsub0, vsub0, plane, 
logo_w, logo_h which means much duplicated code or creation of functions.

- Corrected use of sizeof in av_malloc_array

- changed calculation of the distance regarding exponent, avoid sqrt, 
use "aspect2" variable

- use double *uglarmtable[MAX_SUB + 1][MAX_SUB + 1], 
*uglarmweightsum[MAX_SUB + 1][MAX_SUB + 1]; (instead per 1-dimensional 
array for the planes)

- unconditional av_freep in uninit

- used the alternative loops as suggested by Nicolas (thanks)


Remaining points / answers / questions:

Am 02.01.19 um 16:25 schrieb Nicolas George:
>>>> +                interp = (uint64_t)(interpd /
>>>> +                    uglarmweightsum[(x - logo_x1) - 1 + (y - logo_y1 - 1) * (logo_w - 2)]);
>>> The cast to uint64_t seems suspicious. Can you explain?
>> Every pixel value of the inner logo area is an integer. Only for the
>> calculation of the weighted average, all values use floats. At the end, it
>> is rounded (truncated) to an int.
> int and uint64_t are not the same thing. Why uint64_t?

"interp" was defined as uint64_t in the original code.

Do you see a problem here? Then let us know.

>>> 	pow(x * x * aspect2 + y * y, exponent / 2);
>> Hm. Again, I'm unsure if this "optimization" is worth it. I wouldn't do this
>> normally.
> In this particular instance, definitely yes. Compilers have very little
> latitude to optimize floating point operations, and they will certainly
> not optimize mathematical functions based on subtle arithmetic
> properties. This is a C compiler, not a TI-89.

We have obviously distinct opinions here. I would leave the code because 
it's easier to understand (as written), and it runs once, taking maybe 
some microseconds more for a usual conversion of video taking seconds to 
hours.

But I have no problem changing it. - Done.


Question to "aspect2": is ^2 not good (slow) or something, or why not 
directly use

double aspect2 = ((double)sar.num / sar.den) ^ 2;


> [...]
>      av_assert2(table_t == uglarmtable + (logo_w - x) + table_stride * y);
>      av_assert2(table_b == uglarmtable + (logo_w - x) + table_stride * (logo_h - y - 1));
>      av_assert2(table_l == uglarmtable + table_stride * (logo_h - y - 1) + x);
>      av_assert2(table_r == uglarmtable + table_stride * (logo_h - y - 1) + logo_w - x - 1;
>
> That makes more lines, but the lines are way simpler: no tricky
> arithmetic, all blocks almost identical, with the changes easy to see
> and understand.

I took over these alternative loops for the calculations.

Question to the assert: Is this useful (compared to the running time)? I 
mean, basically it is the same expression as over the loops, only x and 
y are different by the amount the loops are counting them up. I wouldn't 
say that it is probable that one makes an error coding the loop counter 
or that it doesn't somehow function.

Is there another thing which this assert checks that I didn't see?

Regards,
Uwe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avfilter-vf_delogo-add-uglarm-interpolation-mode.patch
Type: text/x-patch
Size: 15658 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190102/a069ce81/attachment.bin>