diff mbox series

[FFmpeg-devel] avfilter/vf_w3fdif: 255 * 256 * 128 -> max

Message ID 1597159169-2380-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel] avfilter/vf_w3fdif: 255 * 256 * 128 -> max
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang Aug. 11, 2020, 3:19 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

max is initialized to ((1 << depth) - 1) * 256 * 128 before.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vf_w3fdif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul B Mahol Aug. 11, 2020, 3:24 p.m. UTC | #1
NAK

On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
>
> max is initialized to ((1 << depth) - 1) * 256 * 128 before.
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavfilter/vf_w3fdif.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavfilter/vf_w3fdif.c b/libavfilter/vf_w3fdif.c
> index 5d64dbd..0f9efbf 100644
> --- a/libavfilter/vf_w3fdif.c
> +++ b/libavfilter/vf_w3fdif.c
> @@ -163,7 +163,7 @@ static void filter_scale(uint8_t *out_pixel, const
> int32_t *work_pixel, int line
>      int j;
>
>      for (j = 0; j < linesize; j++, out_pixel++, work_pixel++)
> -        *out_pixel = av_clip(*work_pixel, 0, 255 * 256 * 128) >> 15;
> +        *out_pixel = av_clip(*work_pixel, 0, max) >> 15;
>  }
>
>  static void filter16_simple_low(int32_t *work_line,
> --
> 1.8.3.1
>
> _______________________________________________
> 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".


Change is pointless and give no real benefits.
Limin Wang Aug. 11, 2020, 3:33 p.m. UTC | #2
On Tue, Aug 11, 2020 at 05:24:06PM +0200, Paul B Mahol wrote:
> NAK
> 
> On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > max is initialized to ((1 << depth) - 1) * 256 * 128 before.
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavfilter/vf_w3fdif.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_w3fdif.c b/libavfilter/vf_w3fdif.c
> > index 5d64dbd..0f9efbf 100644
> > --- a/libavfilter/vf_w3fdif.c
> > +++ b/libavfilter/vf_w3fdif.c
> > @@ -163,7 +163,7 @@ static void filter_scale(uint8_t *out_pixel, const
> > int32_t *work_pixel, int line
> >      int j;
> >
> >      for (j = 0; j < linesize; j++, out_pixel++, work_pixel++)
> > -        *out_pixel = av_clip(*work_pixel, 0, 255 * 256 * 128) >> 15;
> > +        *out_pixel = av_clip(*work_pixel, 0, max) >> 15;
> >  }
> >
> >  static void filter16_simple_low(int32_t *work_line,
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > 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".
> 
> 
> Change is pointless and give no real benefits.

1. max is input as parameter and calculated already, why use it is pointless?
2. keep 8 bit and 16bit with same code so that we can remove the duplicate code
later.
Paul B Mahol Aug. 11, 2020, 3:41 p.m. UTC | #3
On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
> On Tue, Aug 11, 2020 at 05:24:06PM +0200, Paul B Mahol wrote:
>> NAK
>>
>> On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
>> > From: Limin Wang <lance.lmwang@gmail.com>
>> >
>> > max is initialized to ((1 << depth) - 1) * 256 * 128 before.
>> >
>> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > ---
>> >  libavfilter/vf_w3fdif.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/libavfilter/vf_w3fdif.c b/libavfilter/vf_w3fdif.c
>> > index 5d64dbd..0f9efbf 100644
>> > --- a/libavfilter/vf_w3fdif.c
>> > +++ b/libavfilter/vf_w3fdif.c
>> > @@ -163,7 +163,7 @@ static void filter_scale(uint8_t *out_pixel, const
>> > int32_t *work_pixel, int line
>> >      int j;
>> >
>> >      for (j = 0; j < linesize; j++, out_pixel++, work_pixel++)
>> > -        *out_pixel = av_clip(*work_pixel, 0, 255 * 256 * 128) >> 15;
>> > +        *out_pixel = av_clip(*work_pixel, 0, max) >> 15;
>> >  }
>> >
>> >  static void filter16_simple_low(int32_t *work_line,
>> > --
>> > 1.8.3.1
>> >
>> > _______________________________________________
>> > 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".
>>
>>
>> Change is pointless and give no real benefits.
>
> 1. max is input as parameter and calculated already, why use it is
> pointless?
> 2. keep 8 bit and 16bit with same code so that we can remove the duplicate
> code
> later.

Code is not duplicated at all. Different types are used.
You can off course use macros if you are really bored but regressions
in performance are unacceptable.
Your patch also ruin performance.

>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> 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".
Limin Wang Aug. 11, 2020, 4:18 p.m. UTC | #4
On Tue, Aug 11, 2020 at 05:41:58PM +0200, Paul B Mahol wrote:
> On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
> > On Tue, Aug 11, 2020 at 05:24:06PM +0200, Paul B Mahol wrote:
> >> NAK
> >>
> >> On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
> >> > From: Limin Wang <lance.lmwang@gmail.com>
> >> >
> >> > max is initialized to ((1 << depth) - 1) * 256 * 128 before.
> >> >
> >> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >> > ---
> >> >  libavfilter/vf_w3fdif.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/libavfilter/vf_w3fdif.c b/libavfilter/vf_w3fdif.c
> >> > index 5d64dbd..0f9efbf 100644
> >> > --- a/libavfilter/vf_w3fdif.c
> >> > +++ b/libavfilter/vf_w3fdif.c
> >> > @@ -163,7 +163,7 @@ static void filter_scale(uint8_t *out_pixel, const
> >> > int32_t *work_pixel, int line
> >> >      int j;
> >> >
> >> >      for (j = 0; j < linesize; j++, out_pixel++, work_pixel++)
> >> > -        *out_pixel = av_clip(*work_pixel, 0, 255 * 256 * 128) >> 15;
> >> > +        *out_pixel = av_clip(*work_pixel, 0, max) >> 15;
> >> >  }
> >> >
> >> >  static void filter16_simple_low(int32_t *work_line,
> >> > --
> >> > 1.8.3.1
> >> >
> >> > _______________________________________________
> >> > 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".
> >>
> >>
> >> Change is pointless and give no real benefits.
> >
> > 1. max is input as parameter and calculated already, why use it is
> > pointless?
> > 2. keep 8 bit and 16bit with same code so that we can remove the duplicate
> > code
> > later.
> 
> Code is not duplicated at all. Different types are used.
> You can off course use macros if you are really bored but regressions
> in performance are unacceptable.
> Your patch also ruin performance.

I haven't do performance testing for the filter have 8bit x86 asm code, please
ignore it if it'll impact the performance. 

> 
> >
> > --
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
Paul B Mahol Aug. 11, 2020, 4:20 p.m. UTC | #5
On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
> On Tue, Aug 11, 2020 at 05:41:58PM +0200, Paul B Mahol wrote:
>> On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
>> > On Tue, Aug 11, 2020 at 05:24:06PM +0200, Paul B Mahol wrote:
>> >> NAK
>> >>
>> >> On 8/11/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote:
>> >> > From: Limin Wang <lance.lmwang@gmail.com>
>> >> >
>> >> > max is initialized to ((1 << depth) - 1) * 256 * 128 before.
>> >> >
>> >> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> >> > ---
>> >> >  libavfilter/vf_w3fdif.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/libavfilter/vf_w3fdif.c b/libavfilter/vf_w3fdif.c
>> >> > index 5d64dbd..0f9efbf 100644
>> >> > --- a/libavfilter/vf_w3fdif.c
>> >> > +++ b/libavfilter/vf_w3fdif.c
>> >> > @@ -163,7 +163,7 @@ static void filter_scale(uint8_t *out_pixel,
>> >> > const
>> >> > int32_t *work_pixel, int line
>> >> >      int j;
>> >> >
>> >> >      for (j = 0; j < linesize; j++, out_pixel++, work_pixel++)
>> >> > -        *out_pixel = av_clip(*work_pixel, 0, 255 * 256 * 128) >> 15;
>> >> > +        *out_pixel = av_clip(*work_pixel, 0, max) >> 15;
>> >> >  }
>> >> >
>> >> >  static void filter16_simple_low(int32_t *work_line,
>> >> > --
>> >> > 1.8.3.1
>> >> >
>> >> > _______________________________________________
>> >> > 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".
>> >>
>> >>
>> >> Change is pointless and give no real benefits.
>> >
>> > 1. max is input as parameter and calculated already, why use it is
>> > pointless?
>> > 2. keep 8 bit and 16bit with same code so that we can remove the
>> > duplicate
>> > code
>> > later.
>>
>> Code is not duplicated at all. Different types are used.
>> You can off course use macros if you are really bored but regressions
>> in performance are unacceptable.
>> Your patch also ruin performance.
>
> I haven't do performance testing for the filter have 8bit x86 asm code,
> please
> ignore it if it'll impact the performance.

Ah, yes, just write macro for this two scale functions if you care and
makes sense.

>
>>
>> >
>> > --
>> > Thanks,
>> > Limin Wang
>> > _______________________________________________
>> > 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".
>> _______________________________________________
>> 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".
>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavfilter/vf_w3fdif.c b/libavfilter/vf_w3fdif.c
index 5d64dbd..0f9efbf 100644
--- a/libavfilter/vf_w3fdif.c
+++ b/libavfilter/vf_w3fdif.c
@@ -163,7 +163,7 @@  static void filter_scale(uint8_t *out_pixel, const int32_t *work_pixel, int line
     int j;
 
     for (j = 0; j < linesize; j++, out_pixel++, work_pixel++)
-        *out_pixel = av_clip(*work_pixel, 0, 255 * 256 * 128) >> 15;
+        *out_pixel = av_clip(*work_pixel, 0, max) >> 15;
 }
 
 static void filter16_simple_low(int32_t *work_line,