[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [4/7] - G.729 core
Michael Niedermayer
michaelni
Fri Oct 17 01:32:03 CEST 2008
On Sat, Sep 27, 2008 at 03:16:17AM +0700, Vladimir Voroshilov wrote:
> 2008/9/17 Vladimir Voroshilov <voroshil at gmail.com>:
> > 2008/9/17 Michael Niedermayer <michaelni at gmx.at>:
> >> On Sun, Sep 07, 2008 at 07:26:46PM +0700, Vladimir Voroshilov wrote:
> >
> >>> +static const G729_format_description formats[] =
> >>> +{
> >>> + {8000, 10, 160, 13, 0xf892c,},
> >>> +// Note: may not work
> >>> + {4400, 11, 176, 17, 0xf966b,},
> >>> +};
> >>
> >> Is the last parameter really best specified in hex?
> >
> > No differences for me. fixed.
> >
> >>> +/**
> >>> + * \brief Decode LSP coefficients from L0-L3 (3.2.4).
> >>> + * \param lsfq [out] (2.13) decoded LSP coefficients
> >>> + * \param lq_prev [in/out] (2.13) quantized LSF coefficients from previous frames
> >>> + * \param lsf_prev [in/out] (2.13) LSF coefficients from previous frame
> >>> + * \param ma_predictor switched MA predictor of LSP quantizer
> >>> + * \param vq_1st first stage vector of quantizer
> >>> + * \param vq_2nd_low second stage lower vector of LSP quantizer
> >>> + * \param vq_2nd_high second stage higher vector of LSP quantizer
> >>> + */
> >>> +static void g729_lsf_decode(
> >>> + int16_t* lsfq,
> >>> + int16_t lq_prev[MA_NP][10],
> >>> + int16_t* lsf_prev,
> >>> + int16_t ma_predictor,
> >>> + int16_t vq_1st,
> >>> + int16_t vq_2nd_low,
> >>> + int16_t vq_2nd_high)
> >>> +{
> >>> + int i,j,k;
> >>> + static const uint8_t min_distance[2]={10, 5}; //(2.13)
> >>
> >>> + int16_t lq[10]; //(2.13)
> >>
> >> what does "lq" stand for?
> >
> > I've looked into spec more carefully.
> > In this routine variables are (as they called in spec):
> > lsfq[i] - quantized LSF coefficients for the current frame (??(m) in spec)
> > lq[i] - current quantizer output (l?(m) in spec)
> > lq_prev[i] - previous quantizer outputs (l?(m?k) in spec)
> >
> > Hm.. looks like selected names for lq and lq_prev variables are wrong.
> > I sugges renaming (same for restore_from_previous routine):
> > lsf_prev -> "lsfq_prev"
> > lq -> "quantizer_output"
> > lq_prev -> "past_quantizer_outputs"
> >
> >> [...]
> >>> + g729_lq_rotate(lq_prev, lq);
> >>> +
> >>> + ff_acelp_reorder_lsf(lsfq, LSFQ_DIFF_MIN, LSFQ_MIN, LSFQ_MAX, 10);
> >>> +}
> >>> +
> >
> > [...]
> >
> >>> + for(i=0; i<10; i++)
> >>> + {
> >>> + tmp = lsfq[i] << 15;
> >>> +
> >>> + for(k=0; k<MA_NP; k++)
> >>> + tmp -= lq_prev[k][i] * cb_ma_predictor[ma_predictor_prev][k][i];
> >>> +
> >>> + lq[i] = ((tmp >> 15) * cb_ma_predictor_sum_inv[ma_predictor_prev][i]) >> 12;
> >>
> >> the cb_ma_predictor_sum_inv table is unneeded division by cb_ma_predictor_sum
> >> can be used
> >
> > cm_ma_predictor_sum_inv was added especially to avoid division.
> >
> >> also the code looks a little odd, its subtracting here but adding above ...
> >
> > You are talking about lsf_decode and lsf_restore_from previous routines, right?
> >
> > In lsf_decode lsfq[i] is calculated from lq[i]:
> > lsfq[i] = lq[i] * ma_pred_sum[k] + sum[k=0..3] { lq_prev[i][k] * ma_pred[i][k] }
> >
> > lsf_restore_from_previous lq[i] is calculated from lsfq[i]:
> > lq[i] = [ lsfq[i] - sum[k=0..3]{ lq_prev[i][k] * ma_pred[i][k] } ] /
> > ma_pred_sum[k]
> >
> > For some reason this calculation is used instead of simple lq[i] = sum
> > [k=0..2]{lq_prev[i][k] } / 3
> >
> >>> + }
> >>> +
> >>> + g729_lq_rotate(lq_prev, lq);
> >>> +}
> >>
> >> g729_lq_rotate() can be factorized out of these 2 functions that are only
> >> called by if/else
> >
> > In this case lq[i] should be moved outside from g729_lsf_decode and
> > g729_lsf_restore_from_previous routines (while it is used only there)
> > to upper level g729_decode_frame routine
>
> Since i didn't receive any aswer, here is updated patch
> with fixed hex constants and renamed variables.
> g729_lq_rotate placement is not changed.
>
>
>
> --
> Regards,
> Vladimir Voroshilov mailto:voroshil at gmail.com
> JID: voroshil at gmail.com, voroshil at jabber.ru
> ICQ: 95587719
> From 67487e5d616408d6e98b9073c5bc4f250cd7363d Mon Sep 17 00:00:00 2001
> From: voroshil <voroshil at voroshil-laptop.(none)>
> Date: Wed, 3 Sep 2008 12:05:50 +0700
> Subject: [PATCH] G729 decoder
>
>
> diff --git ffmpeg-r15432.orig/libavcodec/g729dec.c ffmpeg-r15432.mod/libavcodec/g729dec.c
> index 6bd814f..74b0665 100644
> --- ffmpeg-r15432.orig/libavcodec/g729dec.c
> +++ ffmpeg-r15432.mod/libavcodec/g729dec.c
> @@ -30,6 +30,9 @@
> #include "libavutil/avutil.h"
> #include "bitstream.h"
>
> +#define FRAC_BITS 15
> +#include "mathops.h"
> +
> #include "g729.h"
> #include "lsp.h"
> #include "acelp_math.h"
this is unflexible and confusing, the used shift should not be file global
[...]
> +static const G729_format_description formats[] =
> +{
> + {8000, 10, 160, 13, 1018156},
> +// Note: may not work
> + {4400, 11, 176, 17, 1021547},
> +};
elaborate on "may" please
if it does not work, it probably should be removed from the patch
or fixed so it does work.
if it does work what does the comment mean?
and if it has not been tested, test it.
[...]
> @@ -138,6 +531,108 @@ static inline int g729_get_parity(uint8_t value)
> 14,
> ctx->subframe_size);
>
> + memcpy(synth, ctx->syn_filter_data, 10 * sizeof(int16_t));
there are 2 memcpies copying this around, i dont see why it would need 2.
> + if(ff_acelp_lp_synthesis_filter(
> + synth+10,
> + &lp[i][1],
> + ctx->exc + i * ctx->subframe_size,
> + ctx->subframe_size,
> + 10,
> + 1,
> + 0x800))
> + {
> + /* Overflow occured, downscale excitation signal... */
> + for(j=0; j<2*MAX_SUBFRAME_SIZE+PITCH_DELAY_MAX+INTERPOL_LEN; j++)
> + ctx->exc_base[j] >>= 2;
is this correct for both subframes?
> +
> + ff_acelp_lp_synthesis_filter(
> + synth+10,
> + &lp[i][1],
> + ctx->exc + i * ctx->subframe_size,
> + ctx->subframe_size,
> + 10,
> + 0,
> + 0x800);
> + }
> + /* Save data (without postfilter) for use in next subframe. */
> + memcpy(ctx->syn_filter_data, synth+ctx->subframe_size, 10 * sizeof(int16_t));
[...]
> + /* Save signal for use in next frame. */
"signal" and "data" are not acceptable descriptions, these 2 words are just
random.
> + memmove(ctx->exc_base, ctx->exc_base + 2 * ctx->subframe_size, (PITCH_DELAY_MAX+INTERPOL_LEN)*sizeof(int16_t));
> +}
> +
> +/**
> + * \brief Decodes one G.729 frame (10 bytes long) into parameter vector.
> + * \param format used format (8k/4.4k/etc)
> + * \param buf 10 bytes of decoder parameters
> + * \param buf_size size of input buffer
> + * \param parm [out] decoded codec parameters
> + *
> + * \return 1 if frame erasure detected, 0 - otherwise
> + */
> +static int g729_bytes2parm(int format, const uint8_t *buf, int buf_size, G729_parameters *parm)
> +{
You mix in this description the word parameter and frame completely
randomly, which considering this is said to convert a frame to parameters
just cause the reader to waste time as he will try to make sense of it until
he realizes that there is no sense in this and that reading the source is
needed.
besides format being what? the comment gives no hint on what to set it to
then buf is said to be 10 bytes and buf_size is ?
I really dont know what i should do with your g729 patches, they need
VERY heavy cleanup and the code that is already in svn probably still needs
work.
I dont really want to let these patches lay around with no review but as they
are they need to be reviewed and cleaned up by the author or someone who knows
g729 well enough to spot all bugs and suboptimal code without having to cross
check the spec & reference implementation.
You are not doing that as it seems, instead you wait until i painstakigly
reverse engeneer each loop and function, and list all contradictions, bugs
and obviously suboptimal code one by one.
Which is made even harder by having most of the comments being plain wrong
I mean seriously i speak no russian but the comments could be in russian it
wouldnt make a difference ;)
So if you could read through your own code and try to clean it up, and
fix comments that are wrong or unneeded or unclear, this would tremendously
help! And it also would speed up review, because the cleaner the code, the
fewer things are there i will complain about and the cleaner it is the
better it can be understood whic means faster review cycles.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/20081017/ab0df5e4/attachment.pgp>
More information about the ffmpeg-devel
mailing list