[FFmpeg-devel] [PATCH] avfilter/vf_interlace: add complex vertcal lowpassfilter

[Thomas Mundt]

> James Almer <jamrial at gmail.com> schrieb am Sa, 11.3.2017:
>>On 3/11/2017 12:39 PM, Thomas Mundt wrote:
>>> James Almer <jamrial at gmail.com> schrieb am Fr, 10.3.2017:
>>>> On 3/8/2017 1:58 PM, Thomas Mundt wrote:
>>>> Hi,
>>>>
>>>> attached patch adds a complex (-1 2 6 2 -1) vertcal lowpassfilter to vf_interlace. This will better retain detail and reduce blurring compared to the existing (1 2 1) filter.
>>>>
>>>> Please comment.
>>>> diff --git a/libavfilter/interlace.h b/libavfilter/interlace.h
>>>> index da073ae..7ad457e 100644
>>>> --- a/libavfilter/interlace.h
>>>> +++ b/libavfilter/interlace.h
>>>> @@ -51,6 +51,8 @@ typedef struct InterlaceContext {
>>>>      AVFrame *cur, *next;   // the two frames from which the new one is obtained
>>>>      void (*lowpass_line)(uint8_t *dstp, ptrdiff_t linesize, const uint8_t *srcp,
>>>>                           const uint8_t *srcp_above, const uint8_t *srcp_below);
>>>> +    void (*lowpass_line_complex)(uint8_t *dstp, ptrdiff_t linesize,
>>>> +                                 const uint8_t *srcp, int mref, int pref);
>>>
>>> Why not keep a single prototype, passing mref and pref for both linear
>>> and complex? You can calculate srcp_above and srcp_below for linear like
>>> you're doing it for complex in both the c and asm versions.
>>>
>> 
>> This would make sense. I just didn´t wanted to change more than necessary in one patch and I´m not such an expert in simd programming.
>> This is my second attempt ever.
>> Also there is the same function in tinterlace filter that also uses this asm. So I would also need to change the function in tinterlace.
>
> Yes, this would need separate patches. One to change the prototype,
> c and asm versions of the linear function for both filters, then
> another (One per interlace filter or one for both) adding the
> complex lowpass filter.
>
OK, c is fine, but I´m stumbling with asm since I´m a newbie there.
I changed in all files:
lowpass_line_c(uint8_t *dstp, ptrdiff_t linesize,
                       const uint8_t *srcp, const uint8_t *srcp_above,
                       const uint8_t *srcp_below)
to
lowpass_line_c(uint8_t *dstp, ptrdiff_t linesize,
                       const uint8_t *srcp, ptrdiff_t mref, ptrdiff_t pref)

But when I change asm to
cglobal lowpass_line, 5, 5, 7
    add r0, r1
    add r2, r1
    neg r1
    pcmpeqb m6, m6
.loop:
    mova m0, [r2+r1+r3]
    mova m1, [r2+r1+r3+mmsize]
    pavgb m0, [r2+r1+r4]
    pavgb m1, [r2+r1+r4+mmsize]
    pxor m0, m6
    pxor m1, m6
    pxor m2, m6, [r2+r1]
    pxor m3, m6, [r2+r1+mmsize]
    pavgb m0, m2
    pavgb m1, m3
    pxor m0, m6
    pxor m1, m6
    mova [r0+r1], m0
    mova [r0+r1+mmsize], m1
    add r1, 2*mmsize
    jl .loop
REP_RET

I get error: invalid effective address
It works when I use the same scheme as I use in complex asm, but this adds some extra calculations which might not be OK.

>> I could do all this as a subsequent patch. Do you have a suggestion how to deal with the option name of complex filter in tinterlace?
>> There it´s a flag with the names "low_pass_filter" and "vlpf", so I could add "complex_low_pass_filter" and "vclpf"?
>
> With flags you could in theory enable both linear and complex at
> the same time, so you'd need to add code to abort in such cases
> or default to one of them.
> I'd say add a lowpass option similar to vf_interlace's, and leave
> the flags as is.

I´m not sure how to add an additional lowpass option without breaking the flag. I might need a hint.
Simplest to me would be adding the complex flag and when both flags are set using the complex function.
Otherwise the user won´t have set it.