[FFmpeg-devel] [PATCH] TwinVQ decoder

Vitor Sessak vitor1001
Mon Jun 8 22:01:40 CEST 2009


Michael Niedermayer wrote:
> On Mon, May 18, 2009 at 06:32:55PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Wed, May 06, 2009 at 05:09:50PM +0200, Vitor Sessak wrote:
>>>> Vitor Sessak wrote:
>>>>> Hi,
>>>>> I'm running out of ideas on how to improve this code, so I'm submitting 
>>>>> it for review. Among the things I'm not 100% happy with is the windowing 
>>>>> mess and the set_interleave_table_* code duplication. Any suggestion is 
>>>>> welcome.
>>>>> About using dsputils->vector_fmul, it gets two parameters (one for 
>>>>> input/output and other for the window), and I need one that gets 
>>>>> three...
>>>>> Finally, it would be very practical to start the review by the tables 
>>>>> since they are so big that every time I send this patch the message need 
>>>>> moderator approval...
>>>> Ping?
>>>>
>>>> New version attached...
>>> [...]

[...]

>> /**
>>  * Evaluate a single LPC amplitude spectrum envelope coefficient from the line
>>  * spectrum pairs
>>  *
>>  * @param cos_lsp a vector of the cosinus of the LSP values
>>  * @param cos_val cos(PI*i/N) where i is the index of the LPC amplitude
>>  * @return the LPC value
>>  *
>>  * @todo reuse code from vorbis_dec.c: vorbis_floor0_decode
>>  */
>> static float eval_lpc_spectrum(const float *cos_lsp, float cos_val, int size)
>> {
>>     int i;
>>     float a = .5;
>>     float b = .5;
>>
>>     for (i=0; i < size; i += 2) {
>>         a *= 2*cos_val - cos_lsp[i  ];
>>         b *= 2*cos_val - cos_lsp[i+1];
>>     }
>>
>>     a *= a*(2+2*cos_val);
>>     b *= b*(2-2*cos_val);
>>
>>     return .5/(a+b);
>> }
> 
> if iam not too tired the .5 and 2 are useless

It is, but it is like that to make it easier to see in what way it is 
similar to vorbis_floor0_decode().

[...]

>> static void extend_pitch(int a1, const float *a2, float a3, float *a4,
>>                          TwinContext *tctx)
>> {
>>     const ModeTab *mtab = tctx->mtab;
>>     int v5;
>>     int v33;
>>     int v35;
>>     int fcmax_i, fcmin_i;
>>     int i, j;
>>     int basf_step = (1 << mtab->basf_bit) - 1;
>>     int isampf = tctx->avctx->sample_rate/1000;
>>     int ibps = tctx->avctx->bit_rate/(1000 * tctx->avctx->channels);
>>
> 
>>     fcmin_i = (40*2*mtab->size + isampf/2)/isampf;
>>
>>     fcmax_i = (40*6*2*mtab->size + isampf/2)/isampf;
> 
> vertical align
> 
> 
>>     v33 = fcmin_i +  (a1*(fcmax_i - fcmin_i) + basf_step/2)/basf_step;
>>
>>     if ((isampf == 22) && (ibps == 32))
>>         v35 = ((v33 + 800LL)* mtab->pitch_cst * mtab->pit_cb_len * v33 +
>>                200LL * mtab->size*v33)/
>>             (400LL * mtab->size * v33);
> 
> looks like some things can be facored out here

Without making it yet more ugly?

> and, btw vXY are unacceptable variable names (this one must be fixed and
> all of them)
> 
> 
>>     else
>>         v35 = (mtab->pitch_cst * mtab->pit_cb_len * v33)/(400 * mtab->size);
>>
>>     for (i=0; i < v35/2; i++)
>>         a4[i] += a3 * a2[i];
>>
>>     v5 = v35/2;
>>
>>     for (i=0; i < mtab->pit_cb_len; i++) {
>>         int v38 = very_broken_op(v33, i + 1);
>>         for (j = - v35/2; j < (v35 + 1)/2 ; j++) {
>>             a4[j+v38] += a3 * a2[v5];
>>             ++v5;
>>
>>             if (v5 >= mtab->pit_cb_len)
>>                 return;
>>         }
>>     }
>> }
>>
> 
>> static void dec_gain(enum FrameType ftype, float *out, TwinContext *tctx)
>> {
>>     const ModeTab *mtab = tctx->mtab;
>>     int i, j;
>>     int sub = mtab->fmode[ftype].sub;
>>     float step     = AMP_MAX     / ((1 <<     GAIN_BITS) - 1);
>>     float sub_step = SUB_AMP_MAX / ((1 << SUB_GAIN_BITS) - 1);
>>
>>     if (ftype == FT_LONG) {
>>         for (i=0; i < tctx->avctx->channels; i++)
>>             out[i] = (1./(1<<10)) *
>>                 mulawinv(step * .5 + step * get_bits(&tctx->gb, GAIN_BITS),
>>                          AMP_MAX, MU);
>>     } else {
>>         for (i=0; i < tctx->avctx->channels; i++) {
>>             float val = (1./(1<<20)) *
>>                 mulawinv(step *.5 + step * get_bits(&tctx->gb, GAIN_BITS),
>>                          AMP_MAX, MU);
>>
>>             for (j=0; j < sub; j++) {
>>                 out[i*sub + j] =
>>                     val*mulawinv(sub_step*.5 +
>>                                  sub_step* get_bits(&tctx->gb, SUB_GAIN_BITS),
>>                                  SUB_AMP_MAX, MU);
>>             }
>>         }
>>     }
>> }
> 
> inefficient, please loose mulawinv() and the pow() it calls

I've tried a LUT, and it doesn't affect total decoding time in a 
(easily) measurable way. I changed the pow() call to just an exp() 
(without an addition log() if the compiler do his job properly).

>> static void check_lsp(int a1, float *a2, float a3)
>> {
>>     int i;
>>
>>     for (i=0; i < a1; i++)
>>         if (a2[i] - a3 < a2[i-1]) {
> 
> the variable names are unacceptable, a1 and a2 are not even the same type
> 
> 
>>             float tmp =(a2[i-1] - a2[i] + a3)*.5;
>>             a2[i-1] -= tmp;
>>             a2[i] += tmp;
>>         }
>> }
>>
> 
>> static void  check_lsp_sort(int a1, float *a2)
>> {
>>     int i,j;
>>
>>     for (i=0;  i < a1; ++i)
>>         for (j=0; j < a1; j++)
>>             if (a2[j-1] > a2[j])
>>                 FFSWAP(float, a2[j],a2[j-1]);
>> }
> 
> same, also doing a O(n^2) by default sort is unacceptable
> are the values mostly sorted already?

yes

> [...]
>> static void dec_bark_env(const uint8_t *in, int use_hist, int ch, float *out,
>>                          enum FrameType ftype, TwinContext *tctx)
>> {
>>     const ModeTab *mtab = tctx->mtab;
>>     int i,j;
>>     float *hist = tctx->bark_hist[ftype][ch];
>>     float val = ((const float []) {0.4, 0.35, 0.28})[ftype];
>>     int bark_n_coef  = mtab->fmode[ftype].bark_n_coef;
>>     int fw_cb_len = mtab->fmode[ftype].bark_env_size / bark_n_coef;
>>     int pos = 0;
>>
>>     for (i = 0; i < fw_cb_len; i++)
>>         for (j = 0; j < bark_n_coef; j++) {
>>             int idx = j + i*bark_n_coef;
>>             float tmp2 = mtab->fmode[ftype].bark_cb[fw_cb_len*in[j] + i];
>>             float st = (use_hist) ?
>>                 (1. - val) * tmp2 + val*hist[idx] + 1. : tmp2 + 1.;
>>
>>             hist[idx] = tmp2;
>>             if (st < -1.) st = 1.;
>>
>>             while (pos < (mtab->fmode[ftype].bark_tab)[idx])
>>                 out[pos++] = st;
> 
> a memset_float() could come in handy, this one isnt the first case i
> see that would benefit from it

Here or in a shared file?

All comments I didn't answer were fixed.

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: twinvq.c
Type: text/x-csrc
Size: 34772 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090608/a9334fb2/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: twinvq_data.h
Type: text/x-chdr
Size: 12740 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090608/a9334fb2/attachment.h>



More information about the ffmpeg-devel mailing list