[FFmpeg-devel] [PATCH] Move Kaiser-Bessel Derived window to mdct.c

[Michael Niedermayer]

On Fri, Jan 11, 2008 at 11:02:05PM +0000, Robert Swain wrote:
> Hello,
> 
> On 11/01/2008, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Jan 11, 2008 at 12:53:10PM +0000, Robert Swain wrote:
> > > From 2c1ff501a911f61251eb4ccd4e8f90386c178337 Mon Sep 17 00:00:00 2001
> > > From: Robert Swain <rob at opendot.cl>
> > > Date: Fri, 11 Jan 2008 09:06:52 +0000
> > > Subject: [PATCH] Move Kaiser-Bessel Derived window generation to mdct.c
> > >
> > > ---
> > >  libavcodec/ac3dec.c  |   26 +-------------------------
> > >  libavcodec/dsputil.h |    1 +
> > >  libavcodec/mdct.c    |   24 ++++++++++++++++++++++++
> > >  3 files changed, 26 insertions(+), 25 deletions(-)
> > [...]
> > > diff --git a/libavcodec/dsputil.h b/libavcodec/dsputil.h
> > > index d541fe0..8dc5ea1 100644
> > > --- a/libavcodec/dsputil.h
> > > +++ b/libavcodec/dsputil.h
> > > @@ -645,6 +645,7 @@ typedef struct MDCTContext {
> > >      FFTContext fft;
> > >  } MDCTContext;
> > >
> > > +void ff_kbd_window_init(float *window);
> > >  int ff_mdct_init(MDCTContext *s, int nbits, int inverse);
> > >  void ff_imdct_calc(MDCTContext *s, FFTSample *output,
> > >                  const FFTSample *input, FFTSample *tmp);
> > > diff --git a/libavcodec/mdct.c b/libavcodec/mdct.c
> > > index de32752..668c514 100644
> > > --- a/libavcodec/mdct.c
> > > +++ b/libavcodec/mdct.c
> > > @@ -26,6 +26,30 @@
> > >   */
> > >
> > >  /**
> > > + * Generate a Kaiser-Bessel Derived Window.
> > > + */
> >
> > Doxygen comments should be in the header ideally (they are easier to find
> > in the header) especially with installed headers though that doesnt apply
> > here
> 
> It seems going back and editing the incremental commits isn't
> particularly easy so I'm going to give up on git's wonderfulness
> (unless anyone has suggestions for managing this) and approach each
> step one at a time. Back to svn for the moment.
> 
> I've moved the Doxygen comment to dsputil.h but I think it's not
> necessarily immediately obvious from the function name what the
> function is, so I added a regular comment with the same text. I also
> added a Doxygen parameter definition in the comment when moving to
> dsputil.h, I hope this is OK.
> 
> > the remainder of patch #1 looks ok
> 
> See attached. When I know this one is OK, I will resubmit the next.
> 
> > [...]
> >
> > > + * @param window    pointer to window coefficient array
> > > + * @param alpha     determines window shape
> > > + * @param n         half of window length
> >
> > hmm, this sounds like n = half of the length of the window array
> 
> What would you prefer? "half of the window size"? "half of the number
> of window elements"? I don't really know what to suggest if what I
> wrote is easily misinterpreted. It seemed the clearest definition to
> me.

hmm, maybe something like
@param window    pointer to half window


> 
> > > + * @param iter      iterations for Bessel I0 approximation
> > >   */
> > > -void ff_kbd_window_init(float *window)
> > > +void ff_kbd_window_init(float *window, float alpha, int n, int iter)
> > >  {
> >
> > iam for hardcoding the iterations
> 
> I suppose I should test how many iterations are required to achieve
> the maximum precision of float, right?

thats an option

and patch looks ok

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20080112/6426f2af/attachment.pgp>