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

[Vitor Sessak]

Justin Ruggles wrote:
> Vitor Sessak wrote:
>> 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?
> 
> previous test
> -------------
> compression level 8:
> double  -- 13.385s -- 167634457
> no sse2 -- 13.981s -- 167634492
> float   -- 13.857s -- 168444919 = 0.4% larger
> 
> patch   -- 13.985s -- 168435032

Thanks for testing. I've tried with a shorter sample (luckynight.wav) 
and got different results.

> about the same speed as double w/o sse. and compression is about the
> same as float-only.
> 
> I think such a small loss in bitrate in favor of reusability is
> acceptable.

While I agree with that in general, I think that the 0.4% gain is worth 
a few preprocessor directives and the LPC_type hack (at least for my 
taste, the patch I posted in the first message of this thread is not too 
ugly).

-Vitor