[FFmpeg-devel] [PATCH] TwinVQ decoder
Reimar Döffinger
Reimar.Doeffinger
Fri Jul 31 16:41:52 CEST 2009
On Wed, Jul 22, 2009 at 09:16:50AM +0200, Vitor Sessak wrote:
> New version attached. Changes:
>
> - Use DSP functions
> - Do not alloc big buffers on the stack
> - Some random optimizations
> - Diego's cosmetics suggestions
I have not looked at previous reviews...
> /**
> * Parameters and tables that are different for each frame type
> */
> typedef struct {
> } FrameMode;
Used only once, so IMO use only struct FrameMode and don't add a typedef.
> typedef struct {
> const FrameMode fmode[3]; ///< frame type-dependant parameters
I think that const makes little sense, for the static tables it
is const because the overall struct is const, and like this
it would make it impossible to initialize fmode.
> } ModeTab;
I'd avoid the typedef here, too.
> static const ModeTab mode_08_08 = {
> {{ 8, bark_tab_s08_64, 10, tab.fcb08s , 1, 5, tab.cb0808s0, tab.cb0808s1, 18},
> { 2, bark_tab_m08_256, 20, tab.fcb08m , 2, 5, tab.cb0808m0, tab.cb0808m1, 16},
> { 1, bark_tab_l08_512, 30, tab.fcb08l , 3, 6, tab.cb0808l0, tab.cb0808l1, 17}
> },
> 512 , 12, tab.lsp08, 1, 5, 3, 3, tab.shape08 , 8, 28, 20, 6, 40
> };
IMO try to use standard indentation.
> typedef struct TwinContext {
> AVCodecContext *avctx;
> DSPContext dsp;
> const ModeTab *mtab;
> float lsp_hist[2][20]; ///< LSP coefficients of the last frame
> int16_t permut[4][4096];
> float bark_hist[3][2][40]; ///< BSE coefficients of last frame
> float *prev; ///< spectrum of last frame
> MDCTContext mdct_ctx[3];
> GetBitContext gb;
> uint8_t length[4][2]; ///< main codebook stride
> uint8_t length_change[4];
> uint8_t bits_main_spec[2][4][2]; ///< bits for the main codebook
> int bits_main_spec_change[4];
> int n_div[4];
Seems a bit chaotic to me with the MDCT and getBit contexts right
in the middle there...
Also I'd think about using an on-stack GetBitContext instead of having
it in the global context, iit just feels wrong to me like this.
> float *out1; ///< non-interleaved output
out1 IMO is a really bad name.
> static void memset_float(float *buf, float val, int size)
> {
> while (size--)
> *buf++ = val;
> }
Things to check:
Is this performance-relevant?
What are the usual values for size?
Is size possibly always or often a multiple of some power of two?
Should it be inline?
Should the loop be (partially) unrolled?
> static void add_vec(int cont, const float *buf1, const float *buf2, float *buf3)
> {
> while (cont--)
> *buf3++ = *buf1++ + *buf2++;
> }
The same applies here, but you should also check if you can/should use restrict on
the pointer arguments.
> * @param 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
The order argument is not documented, esp. its valid ranges/values.
> /**
> * Evaluates the LPC amplitude spectrum envelope from the line spectrum pairs.
> */
> static void eval_lpcenv(const float *cos_vals, float *lpc, TwinContext *tctx)
Putting the context at the end IMO is strange.
> int i;
> const ModeTab *mtab = tctx->mtab;
> int size_s = mtab->size / mtab->fmode[FT_SHORT].sub;
> int mag_95 = 2 * mtab->fmode[FT_SHORT].sub;
> float *cos_TT = ff_cos_tabs[av_log2(mtab->size)-1];
>
> for (i=0; i < (size_s/2); i++) {
Useless ()
> lpc[i] = eval_lpc_spectrum(cos_vals, cos_TT[mag_95*(2*i+1)],
> mtab->n_lsp);
> lpc[size_s-i-1] = eval_lpc_spectrum(cos_vals, -cos_TT[mag_95*(2*i+1)],
> mtab->n_lsp);
The expression mag_95*(2*i+1) should probably a extra variable thhat gets
updated during the loop instead of recalculated each time.
> static void interpolate(float *out, float v1, float v2, int size)
> {
> int j;
> float step = (v1-v2)/size;
>
> out[0] = v2 + step;
> for (j=1; j < size - 1; j++)
> out[j] = out[j-1] + step;
Why j and not i?
Is out[size-1] intentionally not set?
Also
for (i = 0; i < size - 1; i++) {
v2 += step;
out[i] = v2;
}
Seems simpler to me.
> #define GET_COS(sub, idx, forward, cos_tab, size) \
> ((forward) ? \
> ( cos_tab[ (sub)*(idx)]) : \
> (-cos_tab[2*(size) - (sub)*(idx)]))
IMO unless it gives you real speed issue make that a static inline function,
in the worst case using av_always_inline.
> * @param step the size of a block "siiiibiiii"
> * @param in the cosinus of the LSP data
> */
> static inline void eval_lpcenv_or_interp(float *out, const float *in,
> int size, int step, int sub,
> const ModeTab *mtab, int forward)
Several undocumented arguments.
> // Fill the 'iiiibiiii'
> for (i=step; i < size; i += step) {
> if ((i == size - step) ||
> (out[i + step] + out[i - step] > 1.95*out[i]) ||
> (out[i - step] <= out[i + step])) {
I think this assumes that size is a multiple of step and otherwise
may overread. This kind of thing absolutely _must_ be documented,
though it would be even better to avoid (e.g. using i >= size - step).
Also I think this condition is not particularly readable, it should
at least have a comment explaining it.
Performance-wise, checking for "i == size - step" in each iteration
when it is sure it will only be true the last time is not exactly
optimal either, esp. since I think it doesn't save you much complexity
code-wise anyway...
> static void dequant(float *out, enum FrameType ftype, const int16_t *cb0,
> const int16_t *cb1, int cb_len, TwinContext *tctx)
This and others I think lack doxygen comments.
> int length = tctx->length[ftype][i >= tctx->length_change[ftype]];
>
> int bits = tctx->bits_main_spec[0][ftype]
> [i >= tctx->bits_main_spec_change[ftype]];
IMHO you are packing a bit too much in one line.
Also, using a well-named variable for the result of the "i >= ..." result
will also give the reader a better idea of what you are doing.
> if (bits == 7) {
> if (get_bits1(&tctx->gb))
> sign0 = -1;
> tmp0 = get_bits(&tctx->gb, 6);
> } else
> tmp0 = get_bits(&tctx->gb, bits);
bits = 6;
in the if and get rid of the else IMO.
> if (bits == 7) {
> if (get_bits1(&tctx->gb))
> sign1 = -1;
>
> tmp1 = get_bits(&tctx->gb, 6);
> } else
> tmp1 = get_bits(&tctx->gb, bits);
Same.
> int period = min_period +
> ROUNDED_DIV(period_coef*(max_period - min_period),
> (1 << mtab->ppc_period_bit) - 1);
IMO too complex for a single statement.
> if ((isampf == 22) && (ibps == 32)) {
pointless ().
> // Stupid corner case
> width = ROUNDED_DIV((period + 800)* mtab->peak_per2wid, 400*mtab->size);
The comment is not very helpful.
> for (i=1; i<order; i++)
> if (lsp[i-1] > lsp[i] - min_dist) {
> lsp[i-1] = (lsp[i] + lsp[i-1] - min_dist) * 0.5;
> lsp[i ] = (lsp[i] + lsp[i-1] + min_dist) * 0.5;
> }
Hm. I don't like it too much for some reason...
E.g. if (lsp[i] - lsp[i-1] < min_dist)
would seem more logical.
Also I think an extra variable might reduce possible confusion:
float new_prev = (lsp[i] + lsp[i-1] - min_dist) * 0.5;
float new_cur = (lsp[i] + new_prev - min_dist) * 0.5;
lsp[i-1] = new_prev;
lsp[i] = new_cur;
but not 100% sure.
> static void bubblesort(float *lsp, int lp_order)
> {
> int i,j;
>
> /* sort lsp in ascending order. float bubble agorithm,
> O(n) if data already sorted, O(n^2) - otherwise */
> for (i=0; i<lp_order-1; i++)
> for (j=i; j>=0 && lsp[j] > lsp[j+1]; j--)
> FFSWAP(float, lsp[j], lsp[j+1]);
> }
Well, there is the standard C sort function I think, which might be just as well...
Remainder not fully reviewed....
More information about the ffmpeg-devel
mailing list