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

[Uwe Freese]

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
>