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

Vitor Sessak vitor1001
Sat Aug 30 22:43:17 CEST 2008


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...

-Vitor





More information about the ffmpeg-devel mailing list