[FFmpeg-devel,4/6] avcodec/vp6: use ff_vp4_[hv]_loop_filter_12_c

Submitted by Peter Ross on Jan. 13, 2019, 8:02 p.m.

Details

Message ID 1b353c0b4af069072a12974e1d273171138590f3.1547408762.git.pross@xvid.org
State New
Headers show

Commit Message

Peter Ross Jan. 13, 2019, 8:02 p.m.
---
 libavcodec/vp56.c    | 10 ++++++++++
 libavcodec/vp56.h    |  1 +
 libavcodec/vp56dsp.c | 19 -------------------
 3 files changed, 11 insertions(+), 19 deletions(-)

Comments

Carl Eugen Hoyos Jan. 13, 2019, 8:39 p.m.
2019-01-13 21:02 GMT+01:00, Peter Ross <pross@xvid.org>:
> ---
>  libavcodec/vp56.c    | 10 ++++++++++
>  libavcodec/vp56.h    |  1 +
>  libavcodec/vp56dsp.c | 19 -------------------
>  3 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> index 27b4b8b944..c5c5a9fb65 100644
> --- a/libavcodec/vp56.c
> +++ b/libavcodec/vp56.c
> @@ -33,6 +33,8 @@
>
>  void ff_vp56_init_dequant(VP56Context *s, int quantizer)
>  {
> +    if (s->quantizer != quantizer)
> +        ff_vp3dsp_set_bounding_values(s->bounding_values_array,
> ff_vp56_filter_threshold[quantizer]);
>      s->quantizer = quantizer;
>      s->dequant_dc = ff_vp56_dc_dequant[quantizer] << 2;
>      s->dequant_ac = ff_vp56_ac_dequant[quantizer] << 2;
> @@ -320,9 +322,17 @@ static void vp56_add_predictors_dc(VP56Context *s,
> VP56Frame ref_frame)
>  static void vp56_deblock_filter(VP56Context *s, uint8_t *yuv,
>                                  ptrdiff_t stride, int dx, int dy)
>  {
> +    if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
>      int t = ff_vp56_filter_threshold[s->quantizer];
>      if (dx)  s->vp56dsp.edge_filter_hor(yuv +         10-dx , stride, t);
>      if (dy)  s->vp56dsp.edge_filter_ver(yuv + stride*(10-dy), stride, t);
> +    } else {
> +        int * bounding_values = s->bounding_values_array + 127;
> +        if (dx)
> +            ff_vp4_h_loop_filter_12_c(yuv +         10-dx, stride,
> bounding_values);
> +        if (dy)
> +            ff_vp4_v_loop_filter_12_c(yuv + stride*(10-dy), stride,
> bounding_values);
> +    }
>  }

This is your code now but I wonder why it helps not to merge
this with 1/6.

>  static void vp56_mc(VP56Context *s, int b, int plane, uint8_t *src,
> diff --git a/libavcodec/vp56.h b/libavcodec/vp56.h
> index 70e1d38a83..9b3036895c 100644
> --- a/libavcodec/vp56.h
> +++ b/libavcodec/vp56.h
> @@ -170,6 +170,7 @@ struct vp56_context {
>      int filter_mode;
>      int max_vector_length;
>      int sample_variance_threshold;
> +    DECLARE_ALIGNED(8, int, bounding_values_array)[256];
>
>      uint8_t coeff_ctx[4][64];              /* used in vp5 only */
>      uint8_t coeff_ctx_last[4];             /* used in vp5 only */

> diff --git a/libavcodec/vp56dsp.c b/libavcodec/vp56dsp.c
> index 9f299dc60f..e8d93d6680 100644
> --- a/libavcodec/vp56dsp.c
> +++ b/libavcodec/vp56dsp.c
> @@ -72,27 +72,8 @@ av_cold void ff_vp5dsp_init(VP56DSPContext *s)
>  #endif /* CONFIG_VP5_DECODER */
>
>  #if CONFIG_VP6_DECODER
> -static int vp6_adjust(int v, int t)
> -{
> -    int V = v, s = v >> 31;
> -    V ^= s;
> -    V -= s;
> -    if (V-t-1 >= (unsigned)(t-1))
> -        return v;
> -    V = 2*t - V;
> -    V += s;
> -    V ^= s;
> -    return V;
> -}
> -
> -VP56_EDGE_FILTER(vp6, hor, 1, stride)
> -VP56_EDGE_FILTER(vp6, ver, stride, 1)
> -
>  av_cold void ff_vp6dsp_init(VP56DSPContext *s)
>  {
> -    s->edge_filter_hor = vp6_edge_filter_hor;
> -    s->edge_filter_ver = vp6_edge_filter_ver;

And why are the new functions called "_c" but
are not part of the context?
(Or do I misread the code?)

Carl Eugen
Michael Niedermayer Jan. 13, 2019, 10:55 p.m.
On Mon, Jan 14, 2019 at 07:02:47AM +1100, Peter Ross wrote:
> ---
>  libavcodec/vp56.c    | 10 ++++++++++
>  libavcodec/vp56.h    |  1 +
>  libavcodec/vp56dsp.c | 19 -------------------
>  3 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> index 27b4b8b944..c5c5a9fb65 100644
> --- a/libavcodec/vp56.c
> +++ b/libavcodec/vp56.c
> @@ -33,6 +33,8 @@
>  
>  void ff_vp56_init_dequant(VP56Context *s, int quantizer)
>  {
> +    if (s->quantizer != quantizer)
> +        ff_vp3dsp_set_bounding_values(s->bounding_values_array, ff_vp56_filter_threshold[quantizer]);

maybe ive missed something but this fails to build

libavcodec/vp56.c: In function ‘ff_vp56_init_dequant’:
libavcodec/vp56.c:37:9: error: implicit declaration of function ‘ff_vp3dsp_set_bounding_values’ [-Werror=implicit-function-declaration]
         ff_vp3dsp_set_bounding_values(s->bounding_values_array, ff_vp56_filter_threshold[quantizer]);
         ^
cc1: some warnings being treated as errors
make: *** [libavcodec/vp56.o] Error 1
make: *** Waiting for unfinished jobs....


[...]
Peter Ross Jan. 14, 2019, 8:11 a.m.
On Sun, Jan 13, 2019 at 09:39:57PM +0100, Carl Eugen Hoyos wrote:
> 2019-01-13 21:02 GMT+01:00, Peter Ross <pross@xvid.org>:
> > ---
> >  libavcodec/vp56.c    | 10 ++++++++++
> >  libavcodec/vp56.h    |  1 +
> >  libavcodec/vp56dsp.c | 19 -------------------
> >  3 files changed, 11 insertions(+), 19 deletions(-)

> > +        if (dx)
> > +            ff_vp4_h_loop_filter_12_c(yuv +         10-dx, stride,
> > bounding_values);
> > +        if (dy)
> > +            ff_vp4_v_loop_filter_12_c(yuv + stride*(10-dy), stride,
> > bounding_values);
> > +    }
> >  }
> 
> This is your code now but I wonder why it helps not to merge
> this with 1/6.

fair enough.

> > -    s->edge_filter_hor = vp6_edge_filter_hor;
> > -    s->edge_filter_ver = vp6_edge_filter_ver;
> 
> And why are the new functions called "_c" but
> are not part of the context?
> (Or do I misread the code?)

i will drop the _c.

and for consisistency they probably should be ff_vp3dsp_xxx.

they are not part of the context, because they there are are no
asm alternative functions (yet). it seems like a waste creating
context function pointers that only every point to the one function.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Carl Eugen Hoyos Jan. 14, 2019, 11:33 a.m.
2019-01-14 9:11 GMT+01:00, Peter Ross <pross@xvid.org>:
> On Sun, Jan 13, 2019 at 09:39:57PM +0100, Carl Eugen Hoyos wrote:
>> 2019-01-13 21:02 GMT+01:00, Peter Ross <pross@xvid.org>:
>> > ---
>> >  libavcodec/vp56.c    | 10 ++++++++++
>> >  libavcodec/vp56.h    |  1 +
>> >  libavcodec/vp56dsp.c | 19 -------------------
>> >  3 files changed, 11 insertions(+), 19 deletions(-)
>
>> > +        if (dx)
>> > +            ff_vp4_h_loop_filter_12_c(yuv +         10-dx, stride,
>> > bounding_values);
>> > +        if (dy)
>> > +            ff_vp4_v_loop_filter_12_c(yuv + stride*(10-dy), stride,
>> > bounding_values);
>> > +    }
>> >  }
>>
>> This is your code now but I wonder why it helps not to merge
>> this with 1/6.
>
> fair enough.
>
>> > -    s->edge_filter_hor = vp6_edge_filter_hor;
>> > -    s->edge_filter_ver = vp6_edge_filter_ver;
>>
>> And why are the new functions called "_c" but
>> are not part of the context?
>> (Or do I misread the code?)
>
> i will drop the _c.
>
> and for consisistency they probably should be ff_vp3dsp_xxx.
>
> they are not part of the context, because they there are are no
> asm alternative functions (yet). it seems like a waste creating
> context function pointers that only every point to the one function.

If you plan to add asm functions (or if you believe they should be
added) you could leave the function pointers as they are.

But that's all your decision, Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
index 27b4b8b944..c5c5a9fb65 100644
--- a/libavcodec/vp56.c
+++ b/libavcodec/vp56.c
@@ -33,6 +33,8 @@ 
 
 void ff_vp56_init_dequant(VP56Context *s, int quantizer)
 {
+    if (s->quantizer != quantizer)
+        ff_vp3dsp_set_bounding_values(s->bounding_values_array, ff_vp56_filter_threshold[quantizer]);
     s->quantizer = quantizer;
     s->dequant_dc = ff_vp56_dc_dequant[quantizer] << 2;
     s->dequant_ac = ff_vp56_ac_dequant[quantizer] << 2;
@@ -320,9 +322,17 @@  static void vp56_add_predictors_dc(VP56Context *s, VP56Frame ref_frame)
 static void vp56_deblock_filter(VP56Context *s, uint8_t *yuv,
                                 ptrdiff_t stride, int dx, int dy)
 {
+    if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
     int t = ff_vp56_filter_threshold[s->quantizer];
     if (dx)  s->vp56dsp.edge_filter_hor(yuv +         10-dx , stride, t);
     if (dy)  s->vp56dsp.edge_filter_ver(yuv + stride*(10-dy), stride, t);
+    } else {
+        int * bounding_values = s->bounding_values_array + 127;
+        if (dx)
+            ff_vp4_h_loop_filter_12_c(yuv +         10-dx, stride, bounding_values);
+        if (dy)
+            ff_vp4_v_loop_filter_12_c(yuv + stride*(10-dy), stride, bounding_values);
+    }
 }
 
 static void vp56_mc(VP56Context *s, int b, int plane, uint8_t *src,
diff --git a/libavcodec/vp56.h b/libavcodec/vp56.h
index 70e1d38a83..9b3036895c 100644
--- a/libavcodec/vp56.h
+++ b/libavcodec/vp56.h
@@ -170,6 +170,7 @@  struct vp56_context {
     int filter_mode;
     int max_vector_length;
     int sample_variance_threshold;
+    DECLARE_ALIGNED(8, int, bounding_values_array)[256];
 
     uint8_t coeff_ctx[4][64];              /* used in vp5 only */
     uint8_t coeff_ctx_last[4];             /* used in vp5 only */
diff --git a/libavcodec/vp56dsp.c b/libavcodec/vp56dsp.c
index 9f299dc60f..e8d93d6680 100644
--- a/libavcodec/vp56dsp.c
+++ b/libavcodec/vp56dsp.c
@@ -72,27 +72,8 @@  av_cold void ff_vp5dsp_init(VP56DSPContext *s)
 #endif /* CONFIG_VP5_DECODER */
 
 #if CONFIG_VP6_DECODER
-static int vp6_adjust(int v, int t)
-{
-    int V = v, s = v >> 31;
-    V ^= s;
-    V -= s;
-    if (V-t-1 >= (unsigned)(t-1))
-        return v;
-    V = 2*t - V;
-    V += s;
-    V ^= s;
-    return V;
-}
-
-VP56_EDGE_FILTER(vp6, hor, 1, stride)
-VP56_EDGE_FILTER(vp6, ver, stride, 1)
-
 av_cold void ff_vp6dsp_init(VP56DSPContext *s)
 {
-    s->edge_filter_hor = vp6_edge_filter_hor;
-    s->edge_filter_ver = vp6_edge_filter_ver;
-
     s->vp6_filter_diag4 = ff_vp6_filter_diag4_c;
 
     if (ARCH_ARM)