[FFmpeg-devel] [PATCH 2/2] Add SIPR decoder for 5k0, 6k5 and 8k5 modes
Vitor Sessak
vitor1001
Thu Jan 7 01:47:04 CET 2010
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>
More information about the ffmpeg-devel
mailing list