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

[Justin Ruggles]

Michael Niedermayer wrote:
> On Sat, Aug 30, 2008 at 10:43:17PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Sat, Aug 30, 2008 at 01:42:00PM -0400, 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
>>>>
>>>> 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.
>>> IMO 0.4% compression difference is significant, and i will not approve a
>>> patch that causes 0.4% compression loss
>>> These little differences is what seperates the best from the 5th best
>>> compressor sometimes ...
>> So, is my patch ok until someone find a way to avoid using doubles (if 
>> such a thing is possible)? It should be just a matter of removing the 
>> LPC_type hack to adapt my patch when someone remove the usage of double...
> 
> Id like to wait until i can reproduce the compression loss and am able
> to investigate this more ...

I reproduced it 3 more times with the next 3 samples I tested using:
time ffmpeg -i test.wav -compression_level 8 -y test.flac

milk:
patched     2.640s    25680120    54.794%
svn         2.588s    25322552    55.424%

black:
patched     1.568s    17958269    44.658%
svn         1.516s    17702143    45.447%

neon:
patched     2.216s    26559613    42.997%
svn         2.120s    26286503    43.583%

-Justin