[FFmpeg-devel] AMR-NB decoder

[Vitor Sessak]

Colin McQuillan wrote:
> Attached is a patch for an AMR-NB decoder.
> 
> It is not bit-exact. This makes it tricky to verify, but I have been
> checking that internal parameters match the 3GPP decoder for the AMR
> test sequences. The PSNR between the input and output is 3.90 to 8.42
> which is about the same as the reference decoder. The PSNR between the
> two outputs is between 8.50 and 18.16, which seems quite good.

Just a few comments:

> +static const AMROrder order_MODE_475[95] = {
> +AMR_LSF(0, 7), AMR_LSF(0, 6), AMR_LSF(0, 5), AMR_LSF(0, 4),
> +AMR_LSF(0, 3), AMR_LSF(0, 2), AMR_LSF(0, 1), AMR_LSF(0, 0),
> +AMR_LSF(1, 7), AMR_LSF(1, 6), AMR_LSF(1, 5), AMR_LSF(1, 4),
> +AMR_LSF(1, 3), AMR_LSF(1, 2), AMR_LSF(1, 1), AMR_LSF(1, 0),
> +AMR_PLAG(0, 7), AMR_PLAG(0, 6), AMR_PLAG(0, 5),
> +AMR_PLAG(0, 4), AMR_PLAG(0, 3), AMR_PLAG(0, 2),
> +AMR_PLAG(1, 3), AMR_PLAG(1, 2),
> +AMR_PLAG(2, 3), AMR_PLAG(2, 2),
> +AMR_PLAG(3, 3), AMR_PLAG(3, 2),
> +AMR_PGAIN(0, 0), AMR_PGAIN(0, 1), AMR_PGAIN(0, 2), AMR_PGAIN(0, 3),
> +AMR_PGAIN(2, 0), AMR_PGAIN(2, 1), AMR_PGAIN(2, 2), AMR_PGAIN(2, 3),
> +AMR_LSF(2, 5), AMR_LSF(2, 4), AMR_LSF(2, 2), AMR_LSF(2, 0),

I think this would be much more readable aligned

> +
> +static const float lsf_3_1[256][3] = {
> +{   1.46484 ,   20.0195  ,  -31.9824  }, {  37.5977  ,  -13.6719  , -179.443   },
> +{  44.6777  ,  -15.8691  ,  -64.6973  }, {   2.19727 ,  -51.2695  ,  -88.1348  },
> +{  27.5879  ,  175.293   ,  443.604   }, { 246.582   ,  296.387   ,  384.033   },
> +{ 209.229   ,  325.439   ,  555.664   }, { 201.904   ,  382.813   ,  471.924   },

Here you can do s/ }/}/ and it'll fit in 80 columns

> +static const float lsf_3_3[512][4] = {
> +{  16.3574  ,   -4.15039 ,   16.1133  ,   -2.92969 }, {-412.598   , -141.846   ,  -25.3906  ,  -66.4063  },
> +{-262.695   , -289.551   , -450.439   ,  -91.7969  }, {-278.320   , -226.074   , -102.539   ,  -14.1602  },
> +{ -63.2324  , -160.156   , -276.855   , -135.010   }, { 436.523   ,  299.561   ,  111.084   ,   31.4941  },

Very long lines. Any particular reason not to do one vector per line?

> +/** b60 hamming windowed sinc function coefficients */
> +static const float b60[61] = {
> + 0.898529  ,  0.865051  ,  0.769257  ,  0.624054  ,  0.448639  ,  0.265289   ,  0.0959167 , -0.0412598 ,
> +-0.134338  , -0.178986  , -0.178528  , -0.142609  , -0.0849304 , -0.0205078  ,  0.0369568 ,  0.0773926 ,
> + 0.0955200 ,  0.0912781 ,  0.0689392 ,  0.0357056 ,  0.        , -0.0305481  , -0.0504150 , -0.0570068 ,

Same, unless 8 is some kind of "natural" number of columns for some reason.

> +#include "lsp.h"
> +
> +#include "amrnbdata.h"
> +
> +typedef struct AMRContext {
> +

> +    GetBitContext                        gb;

Only used in a single funcion, can be a local variable.

-Vitor