[FFmpeg-devel] [PATCH 2/2] Add SIPR decoder for 5k0, 6k5 and 8k5 modes

[Ronald S. Bultje]

Hi Vitor,

On Fri, Jan 1, 2010 at 6:52 AM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> Ronald S. Bultje wrote:
>> (Also, why is the last one so low? Shouldn't they be ascending?)
>
> Because in the code it do not take he cos() of the last value, funny
> algorithm they use...

Is it possible to rescale every 9th member of all arrays so that that
multiplication isn't needed anymore? Or is that impossible because
Q-tables are shared between all members of the LSF arrays?

On your new path:

+static void dequant(float *out, const int *idx, const float *cbs[])
+{
+    int i;
+
+    int stride  = 2;
+    int num_vec = 5;

This (between i/stride) newline isn't needed, and I'd in fact put all
assignments on the same line, but that's just me.

+static void lsf_decode_fp(float *lsfnew, float *lsf_history,
+                          const SiprParameters *parm)
[..]
+    ff_set_min_dist_lsf(lsfnew, LSFQ_DIFF_MIN / 4000., LP_FILTER_ORDER - 1);

That value is only used once, you could just adjust the DIFF_MIN directly.

+    lsfnew[9] = FFMIN(lsfnew[LP_FILTER_ORDER - 1], 1.3);

Aha, so there is a max. value for the last one. I thought so. :-). OK,
so here's my suggestion: let's merge this into the ff_set_min_dist()
function. WMAVoice same, and so do others.

+    for (i = 0; i < LP_FILTER_ORDER - 1; i++)
+        lsfnew[i] = cos(M_PI * lsfnew[i]);

You're still multiplying the sf by M_PI (and yes, my WMAVoice code
does the same, although * 2 M_PI). I still think this can be in the
tables (sorry if my previous comment was unclear). It saves a few
dozen multiplications per frame, should be faster.

(Interesting to note that my WMAVoice has a min_diff of LSFQ_DIFF_MIN
/ 8000, i.e. after the multiplications, our distances are the same
(0.00625 * 2 * M_PI).)

+/**
+ * Extracts decoding parameters from the input bitstream.
+ * @param parms          parameters structure
+ * @param pgb            pointer to initialized GetbitContext structure
+ */

Get(B)itContext.

+static void lsp2lpc_sipr(const double *isp, float *Az)

isp -> lsp?

[..]
+    ff_lsp2polyf(isp    , pa, lp_half_order    );
+    ff_lsp2polyf(isp + 1, qa, lp_half_order - 1);
+
+    for (i = 1, j = LP_FILTER_ORDER - 1; i < lp_half_order; i++, j--) {
+        double paf=  pa[i]           *(1 + isp[LP_FILTER_ORDER - 1]);
+        double qaf= (qa[i] - qa[i-2])*(1 - isp[LP_FILTER_ORDER - 1]);
+        Az[i-1]  = (paf + qaf)*0.5;
+        Az[j-1]  = (paf - qaf)*0.5;

Spaces around the *?

Weird function (compared to the acelp version). (No comment here, just
noting that it's weird, maybe that's worth a doxy?)

+static void sipr_decode_lp(float *lsfnew, const float *lsfold, float *Az,
+                           int num_subfr)
[..]
+        for (j = 0; j < LP_FILTER_ORDER; j++)
+            lsfint[j] = lsfold[j] * (1 - t) + t * lsfnew[j];

Isn't this what ff_weighted_vector_sumf() or ff_acelp_interpolatef()
(didn't check, guessing by name) is for?

+static void eval_ir(const float *Az, int pitch_lag, float *freq,
+                    float pitch_sharp_factor)
[..]
+    memset(tmp1 + 11, 0, 37*sizeof(float));

Spaces around the *?

+static void decode_frame(SiprContext *ctx, SiprParameters *params,
+                         float *out_data)
[..]
+    float *impulse_response = ir_buf + LP_FILTER_ORDER;
+    float *synth = ctx->synth_buf + 16; // 16 instead of LP_FILTER_ORDER for
+                                        // memory alignment
+
+    int t0_first = 0;
+    AMRFixed fixed_cb;

Another empty line there - I'd remove it...

+        ff_acelp_interpolatef(excitation, excitation - T0 + (T0_frac <= 0),
+                              ff_b60_sinc, 6,
+                              2*((2 + T0_frac)%3 + 1), LP_FILTER_ORDER,
+                              SUBFR_SIZE);
+
+

Two empty lines, spaces around operators.

+    ctx->dsp.vector_clipf(out_data, synth, -1, 32765./(1<<15), frame_size);

32765? Not 32767?

+static int sipr_decode_frame(AVCodecContext *avctx, void *datap,
+                             int *data_size, AVPacket *avpkt)
[..]
+    if (avpkt->size < (ctx->m.bits_per_frame >> 3)) {
+        av_log(avctx, AV_LOG_ERROR,
+               "Error processing packet: packet size (%d) too small\n",
+               avpkt->size);
+
+        *data_size = 0;
+        return -1;
+    }

I didn't check closely, but just to be sure: is it guaranteed that
this amount of bits is read? can it be higher? You're not using
get_bits_left() anywhere so I'm wondering if there's some way to
overhead (which is bad) the input buffer.

+    if (*data_size < SUBFR_SIZE * ctx->m.subframe_count * 4) {

sizeof(float).

Ronald