[FFmpeg-devel] [PATCH] TwinVQ decoder

Vitor Sessak vitor1001
Sat Aug 1 04:33:40 CEST 2009


Hi, and thanks for the review

On Fri, Jul 31, 2009 at 11:41 AM, Reimar
D?ffinger<Reimar.Doeffinger at gmx.de> wrote:
> 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.

I agree.

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

ok

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

I've tried to archive a good readability without having lines with >
80 chars. Using a stardand indentation would break that. If you find a
better way to indent it, I accept suggestions.

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

agree

>> ? ? float *out1; ? ? ? ? ? ? ? ? ? ? ///< non-interleaved output
>
> out1 IMO is a really bad name.

agree

>> static void memset_float(float *buf, float val, int size)
>> {
>> ? ? while (size--)
>> ? ? ? ? *buf++ = val;
>> }
>
> Things to check:
> Is this performance-relevant?

I don't have access to a dev environment until sunday, but I'd say no.

> What are the usual values for size?

1-100

> Is size possibly always or often a multiple of some power of two?

no

> Should it be inline?
> Should the loop be (partially) unrolled?

If I confirm this is not speed-critical, I don' t think this applies.

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

This should be at least a little speed relevant. The cont value is
always a multiple of 16 and typically >512. Do you think this belongs
to dsputils?

>> ?* @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.

[... several suggestions I agree with ...]

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

bubblesort for this case is O(N) (the inner for never loops more than
once) where standard C sort is O(N log N)...

I'll send a new version of the patch in a couple of days.

-Vitor



More information about the ffmpeg-devel mailing list