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

[Michael Niedermayer]

On Fri, Jan 11, 2008 at 12:53:10PM +0000, Robert Swain wrote:
> Hello,
> 
> On 10/01/2008, Justin Ruggles <justinruggles at bellsouth.net> wrote:
> > Robert Swain wrote:
> > > On 10/01/2008, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > OK, I will post more patches for this later this evening.
> >
> > That sounds good. I know Benjamin has been discussing doing this for a
> > while, but I'm guessing there was not a need to share it until native
> > aac was ready.
> 
> Please see attached. They're git-format-patch generated diffs so I'm
> not immediately sure how to apply them with 'patch' but Michael seems
> keen on this extra information in git diffs from the git thread and I
> agree that the diffstat info is quite informative.
> 
> The first patch moves the ac3dec.c Kaiser-Bessel Derived window
> generation function to mdct.c. The function gets renamed to
> ff_kbd_window_init() as it needs the ff_ prefix and it will be used
> for more than just ac3. Also the call in ac3dec.c was edited
> accordingly and the prototype was added to dsputil.h.
> 
> The second patch extends the flexibility of the function to allow
> variable alpha, window size (well, half window size as I understand
> they're symmetric) and Bessel I0 approximation iterations. The
> prototype in dsputil.h and call in ac3dec.c are edited accordingly.
> 
> The third patch is optional and changes the intermediate variable
> types from double to float allowing the removal of the local_window
> variable.
> 
> Enjoy, and comments are of course welcome. :)
> 
> > >> And IIRC the num of iterations are just a matter of how good the
> > >> approximation should be, what does AAC say about the ierations?
> > >
> > > I think you're right but, from looking at the AAC spec (page 162 of
> > > sp04), all I can see is a sum from 0 to inf in the zeroth order first
> > > kind bessel function I0. I've looked around a bit but unless I'm
> > > missing something, it doesn't say anything about the suggested number
> > > of iterations to use. Maxim used 50 in aac.c, Justin used 100 in
> > > ac3dec.c.
> >
> > 100 is a bit excessive. It could likely be changed to 50 and still match
> >  the specification table. Honestly, I don't even care if it doesn't
> > match exactly...floating-point tables in specs annoy me...:)  If
> > precision matters they should have specified it in fixed-point like all
> > the other AC3 tables.
> 
> I have used 100 in the call in ac3dec.c to avoid changing anything
> unnecessarily. Similarly I'll use 50 in aac.c. If it is decided to
> change it to use more or less iterations, it's a quick alteration to
> make. If you decide that you want to hard code the iterations, that's
> easy enough to do as well.
> 
> Regards,
> Rob

> 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

the remainder of patch #1 looks ok


[...]

> + * @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


> + * @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


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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20080111/703fd062/attachment.pgp>