[FFmpeg-devel] [PATCH] avfilter/vf_bwdif: Remove undesireable spatial preference logic

[FFmpeg-devel] [PATCH] avfilter/vf_bwdif: Remove undesireable spatial preference logic

[Philip Langdale]

bwdif inherited this check from yadif, which was originally supposed to
prefer the spatial predictor if the temporal predictor was too far off.

However, the core bwdif algorithm already accounts for the spatial
predictor, so this additional check actually ends up preferring a worse
value, reducing the overall quality.

This was found by cyanreg while writing bwdif_vulkan, and the visual
improvement is pretty dramatic in some samples. If we agree that this
change is desirable, we should update all implementations.

Signed-off-by: Philip Langdale <philipl@overt.org>
---
 libavfilter/vf_bwdif.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
index 65c617ebb3..441bb11e7b 100644
--- a/libavfilter/vf_bwdif.c
+++ b/libavfilter/vf_bwdif.c
@@ -106,11 +106,6 @@ typedef struct ThreadData {
             interpol = (c + e) >> 1;
 
 #define FILTER2() \
-            if (interpol > d + diff) \
-                interpol = d + diff; \
-            else if (interpol < d - diff) \
-                interpol = d - diff; \
- \
             dst[0] = av_clip(interpol, 0, clip_max); \
         } \
  \
-- 
2.37.2

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_bwdif: Remove undesireable spatial preference logic

[Thomas Mundt]

Hi Philip,

Philip Langdale <philipl@overt.org> schrieb am Fr., 24. März 2023, 23:21:

> bwdif inherited this check from yadif, which was originally supposed to
> prefer the spatial predictor if the temporal predictor was too far off.
>
> However, the core bwdif algorithm already accounts for the spatial
> predictor, so this additional check actually ends up preferring a worse
> value, reducing the overall quality.
>
> This was found by cyanreg while writing bwdif_vulkan, and the visual
> improvement is pretty dramatic in some samples. If we agree that this
> change is desirable, we should update all implementations.
>
> Signed-off-by: Philip Langdale <philipl@overt.org>
> ---
>  libavfilter/vf_bwdif.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
> index 65c617ebb3..441bb11e7b 100644
> --- a/libavfilter/vf_bwdif.c
> +++ b/libavfilter/vf_bwdif.c
> @@ -106,11 +106,6 @@ typedef struct ThreadData {
>              interpol = (c + e) >> 1;
>
>  #define FILTER2() \
> -            if (interpol > d + diff) \
> -                interpol = d + diff; \
> -            else if (interpol < d - diff) \
> -                interpol = d - diff; \
> - \
>

Removing this will make lower thirds and other graphic jump up and down
each frame. It is the main improvement over w3fdif that I have ported from
yadif.
Can you provide samples including still graphics that are improved with
this patch?

Regards,
Thomas

>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_bwdif: Remove undesireable spatial preference logic

[Philip Langdale]

On Sat, 25 Mar 2023 00:02:03 +0100
Thomas Mundt <tmundt75@gmail.com> wrote:

> Hi Philip,
> 
> Philip Langdale <philipl@overt.org> schrieb am Fr., 24. März 2023,
> 23:21:
> 
> > bwdif inherited this check from yadif, which was originally
> > supposed to prefer the spatial predictor if the temporal predictor
> > was too far off.
> >
> > However, the core bwdif algorithm already accounts for the spatial
> > predictor, so this additional check actually ends up preferring a
> > worse value, reducing the overall quality.
> >
> > This was found by cyanreg while writing bwdif_vulkan, and the visual
> > improvement is pretty dramatic in some samples. If we agree that
> > this change is desirable, we should update all implementations.
> >
> > Signed-off-by: Philip Langdale <philipl@overt.org>
> > ---
> >  libavfilter/vf_bwdif.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
> > index 65c617ebb3..441bb11e7b 100644
> > --- a/libavfilter/vf_bwdif.c
> > +++ b/libavfilter/vf_bwdif.c
> > @@ -106,11 +106,6 @@ typedef struct ThreadData {
> >              interpol = (c + e) >> 1;
> >
> >  #define FILTER2() \
> > -            if (interpol > d + diff) \
> > -                interpol = d + diff; \
> > -            else if (interpol < d - diff) \
> > -                interpol = d - diff; \
> > - \
> >  
> 
> Removing this will make lower thirds and other graphic jump up and
> down each frame. It is the main improvement over w3fdif that I have
> ported from yadif.
> Can you provide samples including still graphics that are improved
> with this patch?

Hi Thomas,

Sorry for this thread going unanswered for so long. With the Vulkan
hwaccel stuff sorted out, I wanted to come back to this. I discussed
more with Lynne and we're no longer convinced this change is obviously
desirable. You are right about the jumping behaviour - and although
there are samples (eg: the burosch ones on samples.ffmpeg.org) which
look better with the change, I don't think that's something to over
index on.

Thanks,

--phil
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_bwdif: Remove undesireable spatial preference logic

[Lynne]

Jun 11, 2023, 04:53 by philipl@overt.org:

> On Sat, 25 Mar 2023 00:02:03 +0100
> Thomas Mundt <tmundt75@gmail.com> wrote:
>
>> Hi Philip,
>>
>> Philip Langdale <philipl@overt.org> schrieb am Fr., 24. März 2023,
>> 23:21:
>>
>> > bwdif inherited this check from yadif, which was originally
>> > supposed to prefer the spatial predictor if the temporal predictor
>> > was too far off.
>> >
>> > However, the core bwdif algorithm already accounts for the spatial
>> > predictor, so this additional check actually ends up preferring a
>> > worse value, reducing the overall quality.
>> >
>> > This was found by cyanreg while writing bwdif_vulkan, and the visual
>> > improvement is pretty dramatic in some samples. If we agree that
>> > this change is desirable, we should update all implementations.
>> >
>> > Signed-off-by: Philip Langdale <philipl@overt.org>
>> > ---
>> >  libavfilter/vf_bwdif.c | 5 -----
>> >  1 file changed, 5 deletions(-)
>> >
>> > diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
>> > index 65c617ebb3..441bb11e7b 100644
>> > --- a/libavfilter/vf_bwdif.c
>> > +++ b/libavfilter/vf_bwdif.c
>> > @@ -106,11 +106,6 @@ typedef struct ThreadData {
>> >              interpol = (c + e) >> 1;
>> >
>> >  #define FILTER2() \
>> > -            if (interpol > d + diff) \
>> > -                interpol = d + diff; \
>> > -            else if (interpol < d - diff) \
>> > -                interpol = d - diff; \
>> > - \
>> > 
>>
>> Removing this will make lower thirds and other graphic jump up and
>> down each frame. It is the main improvement over w3fdif that I have
>> ported from yadif.
>> Can you provide samples including still graphics that are improved
>> with this patch?
>>
>
> Hi Thomas,
>
> Sorry for this thread going unanswered for so long. With the Vulkan
> hwaccel stuff sorted out, I wanted to come back to this. I discussed
> more with Lynne and we're no longer convinced this change is obviously
> desirable. You are right about the jumping behaviour - and although
> there are samples (eg: the burosch ones on samples.ffmpeg.org) which
> look better with the change, I don't think that's something to over
> index on.
>

I don't recall agreeing that this is desirable.
bwdif_vulkan still looks better than bwdif or bwdif_cuda (and it's got proper edge handling).
I just forgot to investigate this, that's all.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_bwdif: Remove undesireable spatial preference logic

[Thomas Mundt]

Lynne <dev@lynne.ee> schrieb am So., 11. Juni 2023, 20:11:

> Jun 11, 2023, 04:53 by philipl@overt.org:
>
> > On Sat, 25 Mar 2023 00:02:03 +0100
> > Thomas Mundt <tmundt75@gmail.com> wrote:
> >
> >> Hi Philip,
> >>
> >> Philip Langdale <philipl@overt.org> schrieb am Fr., 24. März 2023,
> >> 23:21:
> >>
> >> > bwdif inherited this check from yadif, which was originally
> >> > supposed to prefer the spatial predictor if the temporal predictor
> >> > was too far off.
> >> >
> >> > However, the core bwdif algorithm already accounts for the spatial
> >> > predictor, so this additional check actually ends up preferring a
> >> > worse value, reducing the overall quality.
> >> >
> >> > This was found by cyanreg while writing bwdif_vulkan, and the visual
> >> > improvement is pretty dramatic in some samples. If we agree that
> >> > this change is desirable, we should update all implementations.
> >> >
> >> > Signed-off-by: Philip Langdale <philipl@overt.org>
> >> > ---
> >> >  libavfilter/vf_bwdif.c | 5 -----
> >> >  1 file changed, 5 deletions(-)
> >> >
> >> > diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
> >> > index 65c617ebb3..441bb11e7b 100644
> >> > --- a/libavfilter/vf_bwdif.c
> >> > +++ b/libavfilter/vf_bwdif.c
> >> > @@ -106,11 +106,6 @@ typedef struct ThreadData {
> >> >              interpol = (c + e) >> 1;
> >> >
> >> >  #define FILTER2() \
> >> > -            if (interpol > d + diff) \
> >> > -                interpol = d + diff; \
> >> > -            else if (interpol < d - diff) \
> >> > -                interpol = d - diff; \
> >> > - \
> >> >
> >>
> >> Removing this will make lower thirds and other graphic jump up and
> >> down each frame. It is the main improvement over w3fdif that I have
> >> ported from yadif.
> >> Can you provide samples including still graphics that are improved
> >> with this patch?
> >>
> >
> > Hi Thomas,
> >
> > Sorry for this thread going unanswered for so long. With the Vulkan
> > hwaccel stuff sorted out, I wanted to come back to this. I discussed
> > more with Lynne and we're no longer convinced this change is obviously
> > desirable. You are right about the jumping behaviour - and although
> > there are samples (eg: the burosch ones on samples.ffmpeg.org) which
> > look better with the change, I don't think that's something to over
> > index on.
> >
>
> I don't recall agreeing that this is desirable.
> bwdif_vulkan still looks better than bwdif or bwdif_cuda (and it's got
> proper edge handling).
>

Could you please prove this claim with some samples? I don't have any
suitable hardware for Vulkan.
Just saw that bwdif_vulkan was added to master. Somehow I didn't see the
commit in the cvs log.

>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".