[FFmpeg-devel] [PATCH] TwinVQ decoder

[Vitor Sessak]

Vitor Sessak wrote:
> 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.

And here it is...

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