[FFmpeg-devel] [PATCH 2/2] mips: Optimization of AAC psychoacoustic model functions

[Michael Niedermayer]

On Thu, Nov 22, 2012 at 02:11:54PM +0100, Bojan Zivkovic wrote:
> Signed-off-by: Bojan Zivkovic <bojan at mips.com>
> ---
>  doc/mips.txt                  |    1 +
>  libavcodec/aacpsy.c           |   80 +++++++++-----
>  libavcodec/mips/aacpsy_mips.h |  230 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 282 insertions(+), 29 deletions(-)
>  create mode 100644 libavcodec/mips/aacpsy_mips.h
> 
> diff --git a/doc/mips.txt b/doc/mips.txt
> index aabdef0..cfc6f17 100644
> --- a/doc/mips.txt
> +++ b/doc/mips.txt
> @@ -48,6 +48,7 @@ Files that have MIPS copyright notice in them:
>        float_dsp_mips.c
>        libm_mips.h
>  * libavcodec/mips/
> +      aacpsy_mips.h
>        ac3dsp_mips.c
>        acelp_filters_mips.c
>        acelp_vectors_mips.c
> diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> index 8a57d86..78ccd5f 100644
> --- a/libavcodec/aacpsy.c
> +++ b/libavcodec/aacpsy.c
> @@ -217,6 +217,10 @@ static const float psy_fir_coeffs[] = {
>      -5.52212e-17 * 2, -0.313819 * 2
>  };
>  
> +#if ARCH_MIPS
> +#   include "mips/aacpsy_mips.h"
> +#endif /* ARCH_MIPS */
> +
>  /**
>   * Calculate the ABR attack threshold from the above LAME psymodel table.
>   */
> @@ -557,25 +561,12 @@ static float calc_reduced_thr_3gpp(AacPsyBand *band, float min_snr,
>      return thr;
>  }
>  
> -/**
> - * Calculate band thresholds as suggested in 3GPP TS26.403
> - */
> -static void psy_3gpp_analyze_channel(FFPsyContext *ctx, int channel,
> -                                     const float *coefs, const FFPsyWindowInfo *wi)
> +#ifndef calc_thr_3gpp
> +static void calc_thr_3gpp(const FFPsyWindowInfo *wi, const int num_bands, AacPsyChannel *pch,
> +                          const uint8_t *band_sizes, const float *coefs)
>  {
> -    AacPsyContext *pctx = (AacPsyContext*) ctx->model_priv_data;
> -    AacPsyChannel *pch  = &pctx->ch[channel];
> -    int start = 0;
>      int i, w, g;
> -    float desired_bits, desired_pe, delta_pe, reduction= NAN, spread_en[128] = {0};
> -    float a = 0.0f, active_lines = 0.0f, norm_fac = 0.0f;
> -    float pe = pctx->chan_bitrate > 32000 ? 0.0f : FFMAX(50.0f, 100.0f - pctx->chan_bitrate * 100.0f / 32000.0f);
> -    const int      num_bands   = ctx->num_bands[wi->num_windows == 8];
> -    const uint8_t *band_sizes  = ctx->bands[wi->num_windows == 8];
> -    AacPsyCoeffs  *coeffs      = pctx->psy_coef[wi->num_windows == 8];
> -    const float avoid_hole_thr = wi->num_windows == 8 ? PSY_3GPP_AH_THR_SHORT : PSY_3GPP_AH_THR_LONG;
> -
> -    //calculate energies, initial thresholds and related values - 5.4.2 "Threshold Calculation"
> +    int start = 0;
>      for (w = 0; w < wi->num_windows*16; w += 16) {
>          for (g = 0; g < num_bands; g++) {
>              AacPsyBand *band = &pch->band[w+g];
> @@ -594,6 +585,47 @@ static void psy_3gpp_analyze_channel(FFPsyContext *ctx, int channel,
>              start += band_sizes[g];
>          }
>      }
> +}
> +#endif /* calc_thr_3gpp */
> +
> +#ifndef psy_hp_filter
> +static void psy_hp_filter(const float *firbuf, float *hpfsmpl)
> +{
> +    int i, j;
> +    for (i = 0; i < AAC_BLOCK_SIZE_LONG; i++) {
> +        float sum1, sum2;
> +        sum1 = firbuf[i + (PSY_LAME_FIR_LEN - 1) / 2];
> +        sum2 = 0.0;
> +        for (j = 0; j < ((PSY_LAME_FIR_LEN - 1) / 2) - 1; j += 2) {
> +            sum1 += psy_fir_coeffs[j] * (firbuf[i + j] + firbuf[i + PSY_LAME_FIR_LEN - j]);
> +            sum2 += psy_fir_coeffs[j + 1] * (firbuf[i + j + 1] + firbuf[i + PSY_LAME_FIR_LEN - j - 1]);
> +        }
> +        /* NOTE: The LAME psymodel expects it's input in the range -32768 to 32768. Tuning this for normalized floats would be difficult. */
> +        hpfsmpl[i] = (sum1 + sum2) * 32768.0f;
> +    }
> +}
> +#endif /* psy_hp_filter */

There are a few problems here.
First, our AAC encoder and its psy model are in not so good shape and
its likely that at some point they will change to get them into better
shape and produce better quality ...

Iam thus not sure its a good idea to optimize any of the related code
before that ...

But independant of that, optimiztaions need to have a clean API
for example one can write a FIR filter that takes pointers
to input, output, filter coefficients and the number of such values.
and write an optimized version for that.

The patch though would just take a random chunk of code and optimize
that, thats very unflexible.
It cant be reused anywhere and it will break with the tiniest change
to the code.
I also see that none of the named constants like PSY_LAME_FIR_LEN
that are used in the C code appear in the MIPS code so any change
to them will break it and this will not even be detected as no
assert or anything check for these assumtations.

You should, if you write code that has extra assumtations beyond the
C code always add asserts and documentation to make sure these
assumtations are checked and documented. Debuging the code without
such checks is certainly not fun.
Avoiding extra assumtations is of course even better

[...]

Thanks
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121122/a4f376d3/attachment.asc>