diff mbox

[FFmpeg-devel,1/2] avcodec/dvbsubdec: Factor a few expressions out of compute_default_clut()

Message ID 20171001161036.6133-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Oct. 1, 2017, 4:10 p.m. UTC 
32% faster loop

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/dvbsubdec.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Clément Bœsch Oct. 1, 2017, 4:25 p.m. UTC | #1 
On Sun, Oct 01, 2017 at 06:10:35PM +0200, Michael Niedermayer wrote:
> 32% faster loop
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/dvbsubdec.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
> index b683109643..6a40f2376f 100644
> --- a/libavcodec/dvbsubdec.c
> +++ b/libavcodec/dvbsubdec.c
> @@ -653,8 +653,9 @@ static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
>      uint8_t list_inv[256];
>      int counttab[256] = {0};
>      int count, i, x, y;
> +    ptrdiff_t ls = rect->linesize[0];

You use this variable in 3 places where the length doesn't matter; please
use a longer name (maybe "lsize", "stride", or even "linesize").

>  
> -#define V(x,y) rect->data[0][(x) + (y)*rect->linesize[0]]
> +#define V(x,y) rect->data[0][(x) + (y)*ls]
>      for (y = 0; y<h; y++) {
>          for (x = 0; x<w; x++) {
>              int v = V(x,y) + 1;
> @@ -665,7 +666,7 @@ static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
>              counttab[v-1] += !!((v!=vl) + (v!=vr) + (v!=vt) + (v!=vb));
>          }
>      }
> -#define L(x,y) list[ rect->data[0][(x) + (y)*rect->linesize[0]] ]

> +#define L(x,y) list[ d[(x) + (y)*ls] ]

since you modify this line, feel free to drop the extra spacing
after and before the [].

>  
>      for (i = 0; i<256; i++) {
>          int scoretab[256] = {0};
> @@ -673,12 +674,13 @@ static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
>          int bestv = 0;
>          for (y = 0; y<h; y++) {
>              for (x = 0; x<w; x++) {
> -                int v = rect->data[0][x + y*rect->linesize[0]];
> +                uint8_t *d = &rect->data[0][x + y*ls];
> +                int v = *d;
>                  int l_m = list[v];
> -                int l_l = x     ? L(x-1, y) : 1;
> -                int l_r = x+1<w ? L(x+1, y) : 1;
> -                int l_t = y     ? L(x, y-1) : 1;
> -                int l_b = y+1<h ? L(x, y+1) : 1;

> +                int l_l = x     ? L(-1, 0) : 1;
> +                int l_r = x+1<w ? L(+1, 0) : 1;
> +                int l_t = y     ? L(0, -1) : 1;
> +                int l_b = y+1<h ? L(0, +1) : 1;

I'd be more comfortable with:

                   int l_l = x     ? L(-1, 0) : 1;
                   int l_r = x+1<w ? L( 1, 0) : 1;
                   int l_t = y     ? L( 0,-1) : 1;
                   int l_b = y+1<h ? L( 0, 1) : 1;

I feel like at some point in the future compilers will complain about
these "(+3)*x"

Should be fine otherwise
Michael Niedermayer Oct. 14, 2017, 4:23 p.m. UTC | #2 
On Sun, Oct 01, 2017 at 06:25:14PM +0200, Clément Bœsch wrote:
> On Sun, Oct 01, 2017 at 06:10:35PM +0200, Michael Niedermayer wrote:
> > 32% faster loop
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/dvbsubdec.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
> > index b683109643..6a40f2376f 100644
> > --- a/libavcodec/dvbsubdec.c
> > +++ b/libavcodec/dvbsubdec.c
> > @@ -653,8 +653,9 @@ static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
> >      uint8_t list_inv[256];
> >      int counttab[256] = {0};
> >      int count, i, x, y;
> > +    ptrdiff_t ls = rect->linesize[0];
> 
> You use this variable in 3 places where the length doesn't matter; please
> use a longer name (maybe "lsize", "stride", or even "linesize").
> 
> >  
> > -#define V(x,y) rect->data[0][(x) + (y)*rect->linesize[0]]
> > +#define V(x,y) rect->data[0][(x) + (y)*ls]
> >      for (y = 0; y<h; y++) {
> >          for (x = 0; x<w; x++) {
> >              int v = V(x,y) + 1;
> > @@ -665,7 +666,7 @@ static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
> >              counttab[v-1] += !!((v!=vl) + (v!=vr) + (v!=vt) + (v!=vb));
> >          }
> >      }
> > -#define L(x,y) list[ rect->data[0][(x) + (y)*rect->linesize[0]] ]
> 
> > +#define L(x,y) list[ d[(x) + (y)*ls] ]
> 
> since you modify this line, feel free to drop the extra spacing
> after and before the [].
> 
> >  
> >      for (i = 0; i<256; i++) {
> >          int scoretab[256] = {0};
> > @@ -673,12 +674,13 @@ static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
> >          int bestv = 0;
> >          for (y = 0; y<h; y++) {
> >              for (x = 0; x<w; x++) {
> > -                int v = rect->data[0][x + y*rect->linesize[0]];
> > +                uint8_t *d = &rect->data[0][x + y*ls];
> > +                int v = *d;
> >                  int l_m = list[v];
> > -                int l_l = x     ? L(x-1, y) : 1;
> > -                int l_r = x+1<w ? L(x+1, y) : 1;
> > -                int l_t = y     ? L(x, y-1) : 1;
> > -                int l_b = y+1<h ? L(x, y+1) : 1;
> 
> > +                int l_l = x     ? L(-1, 0) : 1;
> > +                int l_r = x+1<w ? L(+1, 0) : 1;
> > +                int l_t = y     ? L(0, -1) : 1;
> > +                int l_b = y+1<h ? L(0, +1) : 1;
> 
> I'd be more comfortable with:
> 
>                    int l_l = x     ? L(-1, 0) : 1;
>                    int l_r = x+1<w ? L( 1, 0) : 1;
>                    int l_t = y     ? L( 0,-1) : 1;
>                    int l_b = y+1<h ? L( 0, 1) : 1;
> 
> I feel like at some point in the future compilers will complain about
> these "(+3)*x"
> 
> Should be fine otherwise

will apply with the suggested changes

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
index b683109643..6a40f2376f 100644
--- a/libavcodec/dvbsubdec.c
+++ b/libavcodec/dvbsubdec.c
@@ -653,8 +653,9 @@  static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
     uint8_t list_inv[256];
     int counttab[256] = {0};
     int count, i, x, y;
+    ptrdiff_t ls = rect->linesize[0];
 
-#define V(x,y) rect->data[0][(x) + (y)*rect->linesize[0]]
+#define V(x,y) rect->data[0][(x) + (y)*ls]
     for (y = 0; y<h; y++) {
         for (x = 0; x<w; x++) {
             int v = V(x,y) + 1;
@@ -665,7 +666,7 @@  static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
             counttab[v-1] += !!((v!=vl) + (v!=vr) + (v!=vt) + (v!=vb));
         }
     }
-#define L(x,y) list[ rect->data[0][(x) + (y)*rect->linesize[0]] ]
+#define L(x,y) list[ d[(x) + (y)*ls] ]
 
     for (i = 0; i<256; i++) {
         int scoretab[256] = {0};
@@ -673,12 +674,13 @@  static void compute_default_clut(AVSubtitleRect *rect, int w, int h)
         int bestv = 0;
         for (y = 0; y<h; y++) {
             for (x = 0; x<w; x++) {
-                int v = rect->data[0][x + y*rect->linesize[0]];
+                uint8_t *d = &rect->data[0][x + y*ls];
+                int v = *d;
                 int l_m = list[v];
-                int l_l = x     ? L(x-1, y) : 1;
-                int l_r = x+1<w ? L(x+1, y) : 1;
-                int l_t = y     ? L(x, y-1) : 1;
-                int l_b = y+1<h ? L(x, y+1) : 1;
+                int l_l = x     ? L(-1, 0) : 1;
+                int l_r = x+1<w ? L(+1, 0) : 1;
+                int l_t = y     ? L(0, -1) : 1;
+                int l_b = y+1<h ? L(0, +1) : 1;
                 int score;
                 if (l_m)
                     continue;