[FFmpeg-cvslog] r20485 - in trunk/libavcodec: lsp.c lsp.h qcelpdec.c

[Måns Rullgård]

Vitor Sessak <vitor1001 at gmail.com> writes:

> M?ns Rullg?rd wrote:
>> vitor <subversion at mplayerhq.hu> writes:
>>
>>> Author: vitor
>>> Date: Mon Nov  9 13:06:19 2009
>>> New Revision: 20485
>>>
>>> Log:
>>> Do not hardcode filter order in ff_acelp_lspd2lpc()
>>>
>>> Modified:
>>>    trunk/libavcodec/lsp.c
>>>    trunk/libavcodec/lsp.h
>>>    trunk/libavcodec/qcelpdec.c
>>>
>>> Modified: trunk/libavcodec/lsp.c
>>> ==============================================================================
>>> --- trunk/libavcodec/lsp.c	Mon Nov  9 10:11:35 2009	(r20484)
>>> +++ trunk/libavcodec/lsp.c	Mon Nov  9 13:06:19 2009	(r20485)
>>> @@ -155,20 +155,19 @@ static void lsp2polyf(const double *lsp,
>>>      }
>>>  }
>>>
>>> -void ff_acelp_lspd2lpc(const double *lsp, float *lpc)
>>> +void ff_acelp_lspd2lpc(const double *lsp, float *lpc, int lp_half_order)
>>>  {
>>> -    double pa[6], qa[6];
>>> -    int   i;
>>> +    double pa[lp_half_order+1], qa[lp_half_order+1];
>> Sorry I didn't spot this earlier, but we really should avoid
>> variable-length arrays.  Compilers do the silliest things with them.
>> For instance, gcc will not inline a function with a VLA.
>
> I can vaguely understand why a compiler would do that (at least if the
> caller do not pass the length as a constant).

I understand why.  The implications are still undesirable.

>> Some compilers call malloc() to allocate the array, and still some
>> silently miscompile the code.  That is in addition to being an
>> inherently bad idea.  If the allocation fails, you have no chance
>> in hell of recovering.
>
> I suppose the compiler is trying to work-around stupid coders that
> allocate several MB this way :p

I meant VLAs are a bad idea, whether they use malloc or not.  Stack or
malloc, if you blow it you're dead (if you're lucky).

>> In this case, I would suggest setting a sensible upper limit, maybe 8
>> or 16, and using fixed-size arrays.
>
> Fine by me, patch attached.

Review left to the maintainers.

-- 
M?ns Rullg?rd
mans at mansr.com