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

[Michael Niedermayer]

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

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/a0c494ee/attachment.pgp>