[FFmpeg-devel] [PATCH/RFC] Remove triplication of compute_lpc_coefs() function

[Vitor Sessak]

Justin Ruggles wrote:
> Vitor Sessak wrote:
>> Hi
>>
>> Michael Niedermayer wrote:
>>> On Thu, Aug 28, 2008 at 02:28:50PM +0200, Vitor Sessak wrote:
>>>> Hi all.
>>>>
>>>> I've finally found a more or less clean way of doing $subj. I also
>>>> want to know if it is ok to remove the following useless loop:
>>>>
>>>>> for(i=0; i<max_order; i++) lpc_tmp[i] = 0;
>>> all useless loops can be removed ...
>> Gone.
>>
>>>
>>> Some comments ...
>>> Is the double based code really a problem speedwise? IIRC someone (mans?)
>>> said it was very slow on some ARM, but that considered flac float vs.
>>> double which included the autocorrelation stuff, not just
>>> compute_lpc_coefs() float vs. double IIRC.
>> Even if compute_lpc_coefs() is not speed critical, to convert all
>> input/output buffers to double/float before passing to the function is
>> ugly and slow.
>>
>>> It should be clarified how much time is actually spend in this code when
>>> encoding flac.
>>>
>>> Also iam still curious if all variables must be double to maintain the
>>> good
>>> compression with flac ...
>> The following patch makes the flac encoder use a float version of
>> compute_lpc_coefs() but keeps doubles for the intermediate variables. In
>> my tests I've not seem a worsening of the encoded size, but I'd be glad
>> if someone could test it independently.
>>
>> Index: libavcodec/flacenc.c
>> ===================================================================
>> --- libavcodec/flacenc.c	(revision 15005)
>> +++ libavcodec/flacenc.c	(working copy)
>> @@ -595,7 +595,7 @@
>>   * A Welch window function is applied before calculation.
>>   */
>>  void ff_flac_compute_autocorr(const int32_t *data, int len, int lag,
>> -                              double *autoc)
>> +                              float *autoc)
>>  {
>>      int i, j;
>>      double tmp[len + lag + 1];
> 
> This is not compatible with dsputil, which uses doubles.
> 
> I am not against changing this, but the dsputil functions would need to
> be modified as well.

Yes, I forgot to mention this when sending the patch. I hope this is 
something easy to change (I don't know ASM).

Also, would you mind trying this patch with the same sample you used 
last time to see if it really doesn't worsen compression?

Thanks,
-Vitor