[FFmpeg-devel] [PATCH v3] SH4 mpegaudio decoder optimizations

Michael Niedermayer michaelni
Mon Dec 27 01:08:08 CET 2010


On Sun, Dec 26, 2010 at 07:49:29PM +0100, Guennadi Liakhovetski wrote:
> On Sun, 26 Dec 2010, Michael Niedermayer wrote:
> 
> > On Fri, Dec 17, 2010 at 05:54:56PM +0100, Guennadi Liakhovetski wrote:
> > > Hi all
> > > 
> > > This is version 3 of the following patch:
> > > 
> > > This patch implements several mpegaudio optimizations for SH4 FPU-enabled 
> > > SoCs. Verified to provide more than 40% acceleration, when decoding a 
> > > 128kbps stereo mp3 audio file to /dev/null.
> > > 
> > > Changes listed in the patch header, thanks to all who commented!
> > > 
> > > Thanks
> > > Guennadi
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > >  mathops.h      |    2 +
> > >  mpegaudiodec.c |   26 ++++++++++++---
> > >  sh4/mathops.h  |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 120 insertions(+), 5 deletions(-)
> > > ad802c254a9f25cf487eba7d3cb584112c6c59b9  ff_mpegaudio_sh4-v3.patch
> > > From: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
> > > Subject: [PATCH v3] SH4 mpegaudio decoder optimizations
> > > 
> > > This patch implements several mpegaudio optimizations for SH4 FPU-enabled SoCs.
> > > Verified to provide more than 40% acceleration, when decoding a 128kbps stereo
> > > mp3 audio file to /dev/null.
> > > ---
> > > 
> > > v3:
> > > 1. added a "&" constraint to input-output assembly parameters
> > > 
> > > v2:
> > > 1. fixed copyright year to 2010
> > > 2. consistent lower-case register names
> > > 3. joined inline assembly macros into one
> > > 
> > > diff --git a/libavcodec/mathops.h b/libavcodec/mathops.h
> > > index 4d88ed1..b234ae4 100644
> > > --- a/libavcodec/mathops.h
> > > +++ b/libavcodec/mathops.h
> > > @@ -34,6 +34,8 @@
> > >  #   include "mips/mathops.h"
> > >  #elif ARCH_PPC
> > >  #   include "ppc/mathops.h"
> > > +#elif ARCH_SH4
> > > +#   include "sh4/mathops.h"
> > >  #elif ARCH_X86
> > >  #   include "x86/mathops.h"
> > >  #endif
> > > diff --git a/libavcodec/mpegaudiodec.c b/libavcodec/mpegaudiodec.c
> > > index 769be89..513fa1b 100644
> > > --- a/libavcodec/mpegaudiodec.c
> > > +++ b/libavcodec/mpegaudiodec.c
> > > @@ -584,6 +584,22 @@ static inline int round_sample(int64_t *sum)
> > >      op(sum, (w)[7 * 64], (p)[7 * 64]);    \
> > >  }
> > >  
> > > +#ifndef SUM8_MACS
> > > +#define SUM8_MACS(sum, w, p) SUM8(MACS, sum, w, p)
> > > +#endif
> > > +
> > > +#ifndef SUM8_MLSS
> > > +#define SUM8_MLSS(sum, w, p) SUM8(MLSS, sum, w, p)
> > > +#endif
> > > +
> > > +#ifndef SUM8P2_MACS_MLSS
> > > +#define SUM8P2_MACS_MLSS(sum, sum2, w, w2, p) SUM8P2(sum, MACS, sum2, MLSS, w, w2, p)
> > > +#endif
> > > +
> > > +#ifndef SUM8P2_MLSS_MLSS
> > > +#define SUM8P2_MLSS_MLSS(sum, sum2, w, w2, p) SUM8P2(sum, MLSS, sum2, MLSS, w, w2, p)
> > > +#endif
> > > +
> > >  #define SUM8P2(sum1, op1, sum2, op2, w1, w2, p) \
> > >  {                                               \
> > >      INTFLOAT tmp;\
> > > @@ -666,9 +682,9 @@ static void apply_window_mp3_c(MPA_INT *synth_buf, MPA_INT *window,
> > >  
> > >      sum = *dither_state;
> > >      p = synth_buf + 16;
> > > -    SUM8(MACS, sum, w, p);
> > > +    SUM8_MACS(sum, w, p);
> > >      p = synth_buf + 48;
> > > -    SUM8(MLSS, sum, w + 32, p);
> > > +    SUM8_MLSS(sum, w + 32, p);
> > >      *samples = round_sample(&sum);
> > >      samples += incr;
> > >      w++;
> > 
> > This is ugly
> > You should implement the API not change it in one file and implement the
> > modified API
> 
> This doesn't change any API. This just adds a couple of macros completely 
> internal to this file, no existing users are affected.

We have SUM8() (existing API)
You add SUM8_MACS() / SUM8_MLSS() which are a differnt API to do the same
You optimnize your API and switch mpegaudiodec to the new API

Thus we have 2 different APIs and only one of them has your optimizations,
this case is quite bad and not something i want to have to maintain


> 
> > [...]
> > > +
> > > +#define SH_SUM8_MACS(sum, w, p)                      \
> > > +do {                                                 \
> > > +    register const int32_t *wp = (w), *pp = (p);     \
> > > +    register int32_t tmp = 63 * 4;                   \
> > > +    union {int64_t x; int32_t u32[2];} u =           \
> > > +                    {.x = (sum),};                   \
> > > +    __asm__ volatile(                                \
> > > +    "    lds %[hi], mach\n"                          \
> > > +    "    lds %[lo], macl\n"                          \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 0 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 1 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 2 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 3 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 4 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 5 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 6 */           \
> > > +    "    add %[tmp], %[wp]\n"                        \
> > > +    "    add %[tmp], %[pp]\n"                        \
> > > +    "    mac.l @%[wp]+, @%[pp]+\n" /* 7 */           \
> > > +    "    sts mach, %[hi]\n"                          \
> > > +    "    sts macl, %[lo]"                            \
> > > +    : [hi] "+&r" (u.u32[1]),                         \
> > > +      [lo] "+&r" (u.u32[0]),                         \
> > > +      [wp] "+&r" (wp),                               \
> > > +      [pp] "+&r" (pp)                                \
> > > +    : [tmp] "r" (tmp)                                \
> > > +    : "mach", "macl");                               \
> > > +    sum = u.x;                                       \
> > > +} while (0)
> > 
> > Write the whole loop in asm, leaving it to gcc is not a good idea it can just
> > mess up.
> 
> Sorry, what do you mean? how can it "just mess up?"

Compilers turn C code into asm,they arent very good at it, which is why you can
make things 40% faster. They arent better at doing loops thus speed critical
ones should better be written inside asm instead of C loop around asm


> Actually, what loop do
> you mean? You don't mean the "do {...} while (0)" construct, do you?

this: (iam pretty sure you can do better than gcc)
    for(j=1;j<16;j++) {
        sum2 = 0;
        p = synth_buf + 16 + j;
        SUM8P2(sum, MACS, sum2, MLSS, w, w2, p);
        p = synth_buf + 48 - j;
        SUM8P2(sum, MLSS, sum2, MLSS, w + 32, w2 + 32, p);

        *samples = round_sample(&sum);
        samples += incr;
        sum += sum2;
        *samples2 = round_sample(&sum);
        samples2 -= incr;
        w++;
        w2--;
    }

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101227/bb522702/attachment.pgp>



More information about the ffmpeg-devel mailing list