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

[Justin Ruggles]

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

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.  There are many other things in the FLAC encoder that affect
the bitrate much more dramatically.  You might be able to get float-only
versions of the optimized functions from:

http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2008-March/043079.html

I like the idea of mixing floats and doubles to adjust for speed vs.
precision, but to me it seems unnecessary in this case.

-Justin