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

[Vitor Sessak]

Ronald S. Bultje wrote:
> 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?
>   

It's not possible, they are shared...

> 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.
>   

Removed the newline, but I prefer assignments on different lines.

> +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.
>   

I'm not really happy about this for two reasons:

1- There is no max value for AMR and SIPR 16k
2- Such a function would work in a slightly different way of what would
be logical (it doesn't impose a max distance for the last value):

float in = {1, 5, 6, 7, 12}

ff_set_min_dist_lsf_with_max(in, 2.0, 5, 14.0 /* max */);

result: 1, 3, 6, 8, 12

good sense dictates: 1, 3, 6, 8, 10

> +    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 *?
>   

All done.

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

They use indeed a weird algorithm here, no idea why. Drunk codec designers?

> +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?
>   

Very close to ff_weighted_vector_sumf(), but it needs double-precision
output and single precision input (and ff_acelp_interpolatef() is a very
different beast).

> +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?
>   

All this done.

> +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.
>   

This is a constant bitrate codec, so it always read m.bits_per_frame, no
matter what.

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

done.

New patch attached.

-Vitor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_sipr2_7.diff
Type: text/x-patch
Size: 39795 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100107/ad930d88/attachment.bin>