[FFmpeg-devel] [PATCH] delogo filter: new "uglarm" interpolation mode added
Uwe Freese
uwe.freese at gmx.de
Thu Jan 10 21:52:23 EET 2019
Hello,
just a kind reminder. - What is the next step, is there anything more I
should improve, check, modify,...?
For me, the filter works as intended.
@Nicolas: Can you answer my remaining three questions below, please?
Regards,
Uwe
Am 02.01.19 um 23:34 schrieb 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
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list