[FFmpeg-devel] [PATCH] delogo filter: new "uglarm" interpolation mode added
Uwe Freese
uwe.freese at gmx.de
Thu Jan 3 00:34:21 EET 2019
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>
More information about the ffmpeg-devel
mailing list