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

[Uwe Freese]

Hello again,

may I ask again what the next step is to integrate my delogo patch?

It was some effort to change the implementation in the way as discussed 
here and I really would like that the patch is finally added to the 
repository.

Are there any problems, questions, or other points blocking it?

Please let me know, or please find some time to submit the patch to the 
repo (which I can't do). As already written, I'd continue with the 
discussed indentation patch right after that.

Regards,
Uwe

Am 10.01.19 um 20:52 schrieb 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
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel