[FFmpeg-devel] [PATCH] G.729 LSF decoding

[Michael Niedermayer]

On Tue, Jun 23, 2009 at 01:27:37PM +0700, Vladimir Voroshilov wrote:
> 2009/6/22 Michael Niedermayer <michaelni at gmx.at>:
> > On Mon, Jun 22, 2009 at 05:38:58PM +0700, Vladimir Voroshilov wrote:
> >> 2009/6/22 Michael Niedermayer <michaelni at gmx.at>:
> >> > On Sat, Jun 20, 2009 at 02:23:07PM +0700, Vladimir Voroshilov wrote:
> >> >> >> ?g729data.h | ? 11 +++++++++
> >> >> >> ?g729dec.c ?| ? 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >> ?2 files changed, 81 insertions(+)
> >> >> >> 180ee12ea9d2d6990454e9bc3a25d29828c2e0b2 ?0001-LSF-decoding-routines.161.patch
> >> >> >> From e2e6b6b18255c9d24f9cc0cf60dfbd1c4ccbcef1 Mon Sep 17 00:00:00 2001
> >> >> >> From: Vladimir Voroshilov <voroshil at gmail.com>
> >> >> >> Date: Sat, 6 Jun 2009 08:00:56 +0700
> >> >> >> Subject: [PATCH] LSF decoding routines
> >> >> >>
> >> >> >>
> >> >> >> diff --git ffmpeg-r19218/libavcodec/g729data.h ffmpeg-r19218_v161/libavcodec/g729data.h
> >> >> >> index 796d24e..587db67 100644
> >> >> >> --- ffmpeg-r19218/libavcodec/g729data.h
> >> >> >> +++ ffmpeg-r19218_v161/libavcodec/g729data.h
> >> >> >> @@ -262,4 +262,15 @@ static const int16_t cb_ma_predictor[2][MA_NP][10] = { /* (0.15) */
> >> >> >> ? ? ?{ 3024, ?1592, ? 940, ?1631, ?1723, ?1579, ?2034, ?2084, ?1913, ?2601}
> >> >> >> ? ?}
> >> >> >> ?};
> >> >> >> +
> >> >> >> +/**
> >> >> >> + * ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 3
> >> >> >> + * ff_g720_cb_ma_predictor_sum[j][i] = ?1 - sum ( ff_g729_cb_ma_predictor[k][i] ), j=0..1, i=0..9
> >> >> >> + * ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?k=0
> >> >> >> + */
> >> >> >
> >> >> > this doesnt look correct
> >> >>
> >> >> It does in floating-point.
> >> >
> >> > no it does not, this formula is not correct and this is not a matter of
> >> > scaling some constant
> >> >
> >> >
> >> >> I've changed "1" to "32765" (now it is correct for fixed-point) and added notice
> >> >> why values may slightly differ in array and formula.
> >> >
> >> > I think ive said it many month ago already but if you cannot write
> >> > documentation that matches the code then do NOT write documentation. Wrong
> >> > documentation is far worse than no documentation.
> >> >
> >> > first, this set all entries [j][i] for j=0..1, i=0..9, but j is never
> >> > used in the formula, still [0][i] is very different from [1][i], thats not
> >> > just sloppy, then you used constants like 1 instead of 32765 thats also not
> >> > a little off
> >>
> >> Yes, i really missed third dimension at right side.
> >> With below formula i get about +/-1 differences between formula and
> >> table values.
> >>
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?3
> >> ff_g720_cb_ma_predictor_sum[j][i] = ?32765 - sum (
> >> ff_g729_cb_ma_predictor[j][k][i] ), j=0..1, i=0..9
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? k=0
> >>
> >> > and finally it still doesnt match the table and your solution is just adding
> >> > a note saying that initial calculation was done in floating point, what is
> >> > that supposed to mean? Is summing 16bit values in float giving differnt
> >> > results than integers for you? what kind of cpu is that?
> >>
> >> I'm afraid you don't understand me.
> >> Imagine there is some array ?X[N] (floating-point). S=sum(X[n])
> >> and there is another array X_fp[N] (foxed-point) X_fp[n]= X[n] * FP_BASE
> >> S_fp=sum(X_fp[n])
> >>
> >> I just wanted to say that S and S_fp in above sample can slightly differ.
> >
> > what you wrote in the doxy was
> > "Values in array may slightly differ, since initial calculation was done in floating-point."
> >
> > this is totally backward, you KNOW they differ and i suspect you have no
> > faint clue if any calculations where done in floating point by the ITU
> > comitee to build these tables.
> 
> >
> > That would make it a
> > "Values do differ slightly, we do not know why"
> >
> > If OTOH you do know how ITU build these tables then this should be in the
> > doxy not a formlar that differs by +-2 or something.
> 
> You right, i don't know how did they build this table. But, i can
> guess, that they took
> floating-point version of MA predictor codebook and build it, using
> equation 20 in section 3.2.4 of specification.
> For fixed point version they just convert already built table to fixed-point.
> 
> To check my guessing, i've written another sample program:
> 1. got floating-point version of cb_ma_predictor (named "fg" in reference code)
> 2. got floating-point version of cm_ma_predictor_sum (named "fg_sum"
> in reference code)
> 3. applied floating-point formula to 1 and got exactly 2 (no differences).
> 4. convert "fg_sum" to fixed-point and got exactly my
> cb_ma_predictor_sum (no differences).
> 
> How my comment  should look to make above things clear for others ?

just write the working formular in the doxy, if it needs some float table add
that too in the doxy ...


[...]
>  g729data.h |    5 +++++
>  g729dec.c  |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 133a11de754c464e24b2f2cef022eeabd850744c  0001-LSF-decoding-routines.167.patch
> From 84f17f2ab94b27229767448db0be4980ee299b2a Mon Sep 17 00:00:00 2001
> From: Vladimir Voroshilov <voroshil at gmail.com>
> Date: Sat, 6 Jun 2009 08:00:56 +0700
> Subject: [PATCH 01/25] LSF decoding routines

ok

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

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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/20090624/8e657c88/attachment.pgp>