[FFmpeg-devel] [PATCH] QCELP decoder

[Michael Niedermayer]

On Thu, Oct 09, 2008 at 12:54:17PM -0700, Kenan Gillet wrote:
> Hi,
> On Oct 8, 2008, at 5:46 PM, Michael Niedermayer wrote:
[...]
> >> +    int i;
> >> +    float predictor, tmp_lspf;
> >> +
> >> +    if (q->framerate == RATE_OCTAVE) {
> >> +        q->octave_count++;
> >> +
> >> +        for (i = 0; i < 10; i++) {
> >> +            lspf[i] = (i + 1) / 11.
> >> +                    + (q->lspv[i] ?  QCELP_LSP_SPREAD_FACTOR *  
> >> QCELP_LSP_OCTAVE_PREDICTOR
> >> +                                  : -QCELP_LSP_SPREAD_FACTOR *  
> >> QCELP_LSP_OCTAVE_PREDICTOR);
> >> +        }
> >> +
> >> +        // Check the stability of the LSP frequencies.
> >> +        if (lspf[0] < QCELP_LSP_SPREAD_FACTOR)
> >> +            lspf[0] = QCELP_LSP_SPREAD_FACTOR;
> >> +        for (i = 1; i < 10; i++) {
> >> +            if (lspf[i] < lspf[i-1] + QCELP_LSP_SPREAD_FACTOR)
> >> +                lspf[i] = lspf[i-1] + QCELP_LSP_SPREAD_FACTOR;
> >> +        }
> >> +        if (lspf[9] > 1.0 - QCELP_LSP_SPREAD_FACTOR)
> >> +            lspf[9] = 1.0 - QCELP_LSP_SPREAD_FACTOR;
> >> +        for (i = 9; i > 0; i--) {
> >> +            if (lspf[i-1] > lspf[i] - QCELP_LSP_SPREAD_FACTOR)
> >> +                lspf[i-1] = lspf[i] - QCELP_LSP_SPREAD_FACTOR;
> >> +        }
> >
> > this stuff has to be fixed before the patch can be commited, and yes
> > i would give you a hint how if i knew what is intended.
> 
> I am working on it. Unfortunately, I do not have any background in audio
> filtering so I am trying to make sense of the specs:
> http://www.3gpp2.org/Public_html/Specs/C.S0020-0with3Gcover.pdf
> 
> This particular part is on page 48, in section
> '2.4.3.3.1 Converting the LSP Transmission Codes to LSP Frequencies'
> 
> any help would be appreciated :)

search for 0.02 in code/quantize.c of the referenc decoder


[...]
> > [...]
> >> +/**
> >> + * Apply filter in pitch-subframe steps.
> >> + *
> >> + * @param memory a buffer for the previous state of the filter
> >> + * @param gain array of gain per subframe, each element is between  
> >> 0.0 and 2.0
> >> + * @param lag array of lag per subframe, each element is
> >> + *            between 16 and 143 if its corresponding pfrac is 0,
> >> + *            between 16 and 139 otherwise
> >> + * @param pfrac array of boolean per subframe, 1 if the lag is  
> >> fractional, 0 otherwise
> >> + * @param v_in the input vector of the filter
> >> + * @param v_out the output vector of the filter
> >> + */
> >> +static void do_pitchfilter(float *memory,
> >> +                           const float gain[4], const uint8_t  
> >> *lag, const uint8_t pfrac[4],
> >> +                           float v_out[160], const float  
> >> v_in[160]) {
> >> +    int   i, j, k, index;
> >> +    float hamm_tmp;
> >> +
> >> +    memory += 143; // Lookup must be done starting at the end of  
> >> the buffer.
> >> +
> >> +    for (i = 0; i < 4; i++) {
> >> +        if (gain[i] != 0.0) {
> >> +            index = 40 * i - lag[i];
> >> +            for (k = 0; k < 40; k++) {
> >> +                if (pfrac[i]) { // If is a fractional lag...
> >> +                    hamm_tmp = 0.0;
> >
> >> +                    for (j = -4; j < 4; j++) {
> >> +                        hamm_tmp += qcelp_hammsinc_table[j + 4]
> >> +                                  * (index + j < 0 ? memory[index  
> >> + j] : v_out [index + j]);
> >> +                    }
> >
> > having a check in the innermost loop to switch between
> > 2 arrays is not a good idea. the code should be changed so the
> > data is in the same array if possible
> 
> what about:
>                      for (j = -4; j < 4; j++) {
>                          if (index + j >= 0)
>                              break;
>                          hamm_tmp += qcelp_hammsinc_table[j + 4] *  
> memory[index + j];
>                      }
>                      for (; j < 4; j++) {
>                          hamm_tmp += qcelp_hammsinc_table[j + 4] *  
> v_out [index + j];
>                      }

no, id like to see things in the same array or an explanation why this
is a bad idea which may be of course.


[...]
> > [...]
> >> +/**
> >> + * formant synthesis filter
> >> + *
> >> + * TIA/EIA/IS-733 2.4.3.1 (NOOOOT)
> >
> > NOOOOT ?
> >
> 
> don't know either, removed.

ask reynaldo, i have a bad feeling about just removing comments that are
not understood at all

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20081010/28766ba8/attachment.pgp>