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

[Michael Niedermayer]

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

Is there some way your test can be reproduced? So that people who do care
about these 0.1% things can try to find a better solution ..

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080830/69f80b9b/attachment.pgp>