[FFmpeg-devel] [PATCH] Add AMR-NB decoder, next try

[Ronald S. Bultje]

Hi,

On Sun, Jan 3, 2010 at 12:01 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> Hi, the SoC AMR decoder is in a pretty nice shape and have already gone
> through a few review rounds, so I'll give my try in getting it committed.

+/**
+ * @file libavcodec/amrnbdata.h
+ * AMR narrowband data and definitions
+ */
[..]

Any defines that can be in the C should be in the C (as Diego said).

+/**
+ * Bit-order table type
+ */
+typedef struct AMROrder {
+    uint8_t index;                ///< index in (uint16_t *)AMRNBFrame
+    uint8_t bit;                  ///< bit index, LSB=0, MSB=15
+} AMROrder;

OK? So let's look at what we have below:

+static const AMROrder order_MODE_4k75[95] = {

??? Is this defining a scripting language for the buildup of the frame
here? No wonder the patch is huge. Is there a particular reason we're
doing it? Certainly doesn't seem faster to me...

+    uint16_t sid_vector;       ///< index of SID LSF reference vector
+    uint16_t sid_energy;       ///< SID frame energy

What's an SID?

+// The following order* tables are used to convert AMR frame parameters to and
+// from a bitstream. See 3GPP TS 26.101 for more information.

/**Doxy*/

+#define AMR_BIT(field, bit)                  {offsetof(AMRNBFrame,
field) >> 1, bit}
[..]
+ AMR_SENERGY(   2), AMR_SENERGY(   1), AMR_SENERGY(   0)
[..]

(for all this stuff, see the comment above, I'm not sure this is the
most optimal solution for reading a bitstream.)

+static const AMROrder * const amr_unpacking_bitmaps_per_mode[9] = {

9 -> N_MODES?

+// LSF tables

/** Doxy */

+static const int16_t lsf_3_3_MODE_5k15[128][4] = {
+{  419,  163,  -30, -262},{ -455, -789,-1430, -721},{ 1006,  664,  269,   25},
[..]

<space><space><space><space>{ .. },<space> { .. },<space><etc>

+#define LSF_R_FAC          (8000.0/32768.0) ///< LSF residual tables to Hertz
+#define MIN_LSF_SPACING             50.0488 ///< Ensures stability of
LPC filter

I suppose this means that all tables are in this virtually random
scale as well? Is that so that the tables above (lsf_mean etc.) fit in
int16_t instead of float, or is there another reason for it? If it's
not for int16_t <> float, maybe we want to convert this to the final
scale used for LSF/P -> LPC conversion?

+/** Double version of ff_weighted_vector_sumf() */
+void weighted_vector_sumd(double *out, const double *in_a, const double *in_b,
+                          double weight_coeff_a, double weight_coeff_b,
+                          int length)

Should be shared between others, so not here but in lsp.c or so.

+static av_cold int amrnb_decode_init(AVCodecContext *avctx)
[..]
+    for (i = 0; i < LP_FILTER_ORDER; i++) {
+        p->prev_lsp_sub4[i] = lsp_sub4_init[i] * 1000 / (float)(1 << 15);
+        p->lsf_avg[i]       =
+        p->lsf_q[3][i]      = lsp_avg_init[i]         / (float)(1 << 15);

I'm not a fan of this, easily confused for a coding bug.

+static void lsf2lsp(const float *lsf, double *lsp)
[..]
+{
+    int i;
+
+    for (i = 0; i < LP_FILTER_ORDER; i++)
+        lsp[i] = cos(2.0 * M_PI / 8000.0 * lsf[i]);
+}

So this is sort of what I though, the scale of the LSFs is random and
is converted only just before LSP->LPC conversion. I'm OK with it if
that really makes the tables smaller (i.e. int16_t vs. float), but
only then.

Also, I'm wondering if we shouldn't do lsfs in double also, since
we're artificially adding precision here (= noise).

(I didn't review closely below this because I'm late for work. :-).)

Ronald