On 08/27/2010 11:13 PM, Marcelo Galvão Póvoa wrote:

On 26 August 2010 18:55, Vitor Sessak<vitor1001@gmail.com> wrote:

...

Marcelo Galvão Póvoa wrote:

...

Fixed excitation array incorrect length.

Should now be applicable cleanly to the latest ffmpeg revision (fd151a5f8bd152c456a).

First look:

[...]

...

...

+/** + * Convert an ISF vector into an ISP vector + * + * @param[in] isf Isf vector + * @param[out] isp Output isp vector + * @param[in] size Isf/isp size + */ +static void isf2isp(const float *isf, double *isp, int size) +{ + int i; + + for (i = 0; i< size - 1; i++) + isp[i] = cos(2.0 * M_PI * isf[i]); + + isp[size - 1] = cos(4.0 * M_PI * isf[size - 1]); +}

Almost duplication of amrnbdec.c:lsf2lsp().

Yes, but this last element doubling diverges between AMR-NB and AMR-WB reference sources.

Can't you just do "isp[size - 1] *= 2" or it is used elsewhere?+ f[i] = val * f[i - 1] + 2 * f[i - 2]; [...]

...

...

+ for(j = i - 1; j> 1; j--) + f[j] += f[j - 1] * val + f[j - 2]; + f[1] += 0.25 * val; + } +}

Hmm, can't this function be replaced by ff_lsp2polyf() with some rescaling?

Actually I realize now that the difference between these functions is just the entire output scaled down by 0.25. That was silly because I was scaling by 4.0 after this function. It is weird why the reference decoder does this process. Maybe it should worth someone checking it out to be sure about this.

I suppose you checked that after this change the output of your decoder didn't change, no?

...

...

+/** + * Convert a ISP vector to LP coefficient domain {a_k} + * Equations from TS 26.190 section 5.2.4 + * + * @param[in] isp ISP vector for a subframe + * @param[out] lp LP coefficients + * @param[in] lp_half_order Half the number of LPs to construct + */ +static void isp2lp(const double *isp, float *lp, int lp_half_order) { + double pa[10 + 1], qa[10 + 1]; + double last_isp = isp[2 * lp_half_order - 1]; + double qa_old = 0.0; + float *lp2 =&lp[2 * lp_half_order]; + int i; + + if (lp_half_order> 8) { // high-band specific + lsp2polyf_16k(isp, pa, lp_half_order); + lsp2polyf_16k(isp + 1, qa, lp_half_order - 1); + + for (i = 0; i<= lp_half_order; i++) + pa[i] *= 4.0; + for (i = 0; i< lp_half_order; i++) + qa[i] *= 4.0; + } else { + ff_lsp2polyf(isp, pa, lp_half_order); + ff_lsp2polyf(isp + 1, qa, lp_half_order - 1); + } + + for (i = 1; i< lp_half_order; i++) { + double paf = (1 + last_isp) * pa[i]; + double qaf = (1 - last_isp) * (qa[i] - qa_old); + + qa_old = qa[i - 1]; + + lp[i] = 0.5 * (paf + qaf); + lp2[-i] = 0.5 * (paf - qaf); + } + + lp[0] = 1.0; + lp[lp_half_order] = 0.5 * (1 + last_isp) * pa[lp_half_order]; + lp2[0] = last_isp; +}

Please double-check if this is not a duplication of sipr.c:lsp2lpc_sipr().

Interesting, I believe they are the same function except for the fact that in AMR-WB the first LP coefficient is always 1.0 but this should be easily adapted by passing lp + 1 to the function and setting lp[0] = 1.0. Also, the LP order is different between these codecs. To reuse this existing function it should be moved to another file (lsp.c I guess) and modified. Should I do it and include in the next patch?

You can send directly a patch to ffmpeg-devel to move this code to lsp.c.

...

...

+static void decode_pitch_vector(AMRWBContext *ctx, + const AMRWBSubFrame *amr_subframe, + const int subframe) +{ + int pitch_lag_int, pitch_lag_frac; + int i; + float *exc = ctx->excitation; + enum Mode mode = ctx->fr_cur_mode; + + if (mode<= MODE_8k85) { + decode_pitch_lag_low(&pitch_lag_int,&pitch_lag_frac, amr_subframe->ada p, +&ctx->base_pitch_lag, subframe, mode); + } else + decode_pitch_lag_high(&pitch_lag_int,&pitch_lag_frac, amr_subframe->ad ap, +&ctx->base_pitch_lag, subframe); + + ctx->pitch_lag_int = pitch_lag_int; + pitch_lag_int += (pitch_lag_frac< 0 ? -1 : 0) + (pitch_lag_frac ? 1 : 0); + + + /* Calculate the pitch vector by interpolating the past excitation at the + pitch lag using a hamming windowed sinc function */ + ff_acelp_interpolatef(exc, exc + 1 - pitch_lag_int, + ac_inter, 4, + pitch_lag_frac + (pitch_lag_frac> 0 ? 0 : 4), + LP_ORDER, AMRWB_SFR_SIZE + 1);

ac_inter is yet another hamming function. Can you check if you cannot reuse acelp_vectors.c:ff_b60_sinc or sipr16kdata.h:sinc_win?

Well, I cannot tell precisely if it cannot be reused but I guess so. The number of coefficients in the table is related to the fractional resolution of the interpolation: in AMR-NB and SIPR it is 4 but in AMR-WB it is 6. Also, I don't see similarities between the table I extracted from the reference and these tables.

Ok, so leave it like this by now.

...

...

+/** + * Reduce fixed vector sparseness by smoothing with one of three IR filters + * Also known as "adaptive phase dispersion" + * + * @param[in] ctx The context + * @param[in,out] fixed_vector Unfiltered fixed vector + * @param[out] buf Space for modified vector if necessary + * + * @return The potentially overwritten filtered fixed vector address + */ +static float *anti_sparseness(AMRWBContext *ctx, + float *fixed_vector, float *buf)

amrnbedec.c has a function with the same name. Any code can be reused?

The function skeletons are similar but there are differences everywhere. I can think about easily reusing only about 6 lines, which wouldn't worth much

Ok, leave it as-is then.

...

...

+/** + * Apply to synthesis a 2nd order high-pass filter + * + * @param[out] out Buffer for filtered output + * @param[in] hpf_coef Filter coefficients as used below + * @param[in,out] mem State from last filtering (updated) + * @param[in] in Input speech data + * + * @remark It is safe to pass the same array in in and out parameters + */ +static void high_pass_filter(float *out, const float hpf_coef[2][3], + float mem[4], const float *in) +{ + int i; + float *x = mem - 1, *y = mem + 2; // previous inputs and outputs + + for (i = 0; i< AMRWB_SFR_SIZE; i++) { + float x0 = in[i]; + + out[i] = hpf_coef[0][0] * x0 + hpf_coef[1][0] * y[0] + + hpf_coef[0][1] * x[1] + hpf_coef[1][1] * y[1] + + hpf_coef[0][2] * x[2]; + + y[1] = y[0]; + y[0] = out[i]; + + x[2] = x[1]; + x[1] = x0; + } +}

acelp_filter.c:ff_acelp_apply_order_2_transfer_function()

Are you sure? I can't see them as equivalent, could you explain?

I'll give a better look at it soon...

...

...

+/** + * Upsample a signal by 5/4 ratio (from 12.8kHz to 16kHz) using + * a FIR interpolation filter. Uses past data from before *in address + * + * @param[out] out Buffer for interpolated signal + * @param[in] in Current signal data (length 0.8*o_size) + * @param[in] o_size Output signal length + */ +static void upsample_5_4(float *out, const float *in, int o_size) +{ + const float *in0 = in - UPS_FIR_SIZE + 1; + int i; + + for (i = 0; i< o_size; i++) { + int int_part = (4 * i) / 5; + int frac_part = (4 * i) - 5 * int_part;

You can break this loop in two to avoid the division:

i = 0; for (j = 0; j< o_size/5; j++) for (k = 0; k< 5; k++) { .... i++; }

I've done this in not so neat way, adding 4 and calculating a simple remainder by 5. Tell me if can be done in a better way.

Does something like the following work? int int_part = 0, frac_part = 0; i = 0; for (j = 0; j< o_size / 5; j++) { out[i] = in[int_part]; frac_part = 4; i++; for (k = 1; k< 5; k++) { out[i] = ff_dot_productf(in0 + int_part, upsample_fir[4 - frac_part], UPS_MEM_SIZE); int_part++; frac_part--; i++; } }

...

...

+/** + * Generate the high-band excitation with the same energy from the lower + * one and scaled by the given gain + * + * @param[in] ctx The context + * @param[out] hb_exc Buffer for the excitation + * @param[in] synth_exc Low-band excitation used for synthesis + * @param[in] hb_gain Wanted excitation gain + */ +static void scaled_hb_excitation(AMRWBContext *ctx, float *hb_exc, + const float *synth_exc, float hb_gain) +{ + int i; + float energy = ff_dot_productf(synth_exc, synth_exc, AMRWB_SFR_SIZE); + + /* Generate a white-noise excitation */ + for (i = 0; i< AMRWB_SFR_SIZE_16k; i++) + hb_exc[i] = 32768.0 - (uint16_t) av_lfg_get(&ctx->prng); + + ff_scale_vector_to_given_sum_of_squares(hb_exc, hb_exc, energy, + AMRWB_SFR_SIZE_16k); + + for (i = 0; i< AMRWB_SFR_SIZE_16k; i++) + hb_exc[i] *= hb_gain; +}

Why are you scaling it twice?

The first scaling is to match the energy with the lower band part. The second is the high_band gain itself. This can be done in only one loop using ff_scale_vector_to_given_sum_of_squares() ?

I think you will be obliged to duplicate a little of ff_scale_vector_to_given_sum_of_squares(), but it will make your code faster...

...

...

+/** + * Apply to high-band samples a 15th order filter + * The filter characteristic depends on the given coefficients + * + * @param[out] out Buffer for filtered output + * @param[in] fir_coef Filter coefficients + * @param[in,out] mem State from last filtering (updated) + * @param[in] cp_gain Compensation gain (usually the filter gain) + * @param[in] in Input speech data (high-band) + * + * @remark It is safe to pass the same array in in and out parameters + */ +static void hb_fir_filter(float *out, const float fir_coef[HB_FIR_SIZE + 1], + float mem[HB_FIR_SIZE], float cp_gain, const float *in) +{ + int i, j; + float data[AMRWB_SFR_SIZE_16k + HB_FIR_SIZE]; // past and current samples + + memcpy(data, mem, HB_FIR_SIZE * sizeof(float)); + + for (i = 0; i< AMRWB_SFR_SIZE_16k; i++) + data[i + HB_FIR_SIZE] = in[i] / cp_gain; + + for (i = 0; i< AMRWB_SFR_SIZE_16k; i++) { + out[i] = 0.0; + for (j = 0; j<= HB_FIR_SIZE; j++) + out[i] += data[i + j] * fir_coef[j]; + } + + memcpy(mem, data + AMRWB_SFR_SIZE_16k, HB_FIR_SIZE * sizeof(float)); +}

I think it is cleaner (and more consistent) to do like the synthesis filter and get one single buffer for output and memory...

The problem is that this filter memory consists of past inputs and not outputs. If I would do with a single buffer I would have to copy the input to it in order to avoid overwriting the last filter output.

ok Also

+/** + * Parses a speech frame, storing data in the Context + * + * @param[in,out] ctx The context + * @param[in] buf Pointer to the input buffer + * @param[in] buf_size Size of the input buffer + * + * @return The frame mode + */ +static enum Mode unpack_bitstream(AMRWBContext *ctx, const uint8_t *buf, + int buf_size) +{ + GetBitContext gb; + uint16_t *data; + + init_get_bits(&gb, buf, buf_size * 8); + + /* decode frame header (1st octet) */ + skip_bits(&gb, 1); // padding bit + ctx->fr_cur_mode = get_bits(&gb, 4); + ctx->fr_quality = get_bits1(&gb); + skip_bits(&gb, 2); // padding bits + + // XXX: We are using only the "MIME/storage" format + // used by libopencore. This format is simpler and + // does not carry the auxiliary information of the frame + + data = (uint16_t *)&ctx->frame; + memset(data, 0, sizeof(AMRWBFrame)); + buf++; + + if (ctx->fr_cur_mode< MODE_SID) { /* Normal speech frame */ + const uint16_t *perm = amr_bit_orderings_by_mode[ctx->fr_cur_mode]; + int field_size; + + while ((field_size = *perm++)) { + int field = 0; + int field_offset = *perm++; + while (field_size--) { + uint16_t bit_idx = *perm++; + field<<= 1; + /* The bit index inside the byte is reversed (MSB->LSB) */ + field |= BIT_POS(buf[bit_idx>> 3], 7 - (bit_idx& 7)); + } + data[field_offset] = field; + } + } + + return ctx->fr_cur_mode; +}

Can't you reorder the bit indexes in a way it is in the LSB->MSB so you can use just the usual bitstream functions? -Vitor

On 30 August 2010 09:31, Vitor Sessak <vitor1001@gmail.com> wrote:

On 08/27/2010 11:13 PM, Marcelo Galvão Póvoa wrote:

...

On 26 August 2010 18:55, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

Marcelo Galvão Póvoa wrote:

...

Fixed excitation array incorrect length.

Should now be applicable cleanly to the latest ffmpeg revision (fd151a5f8bd152c456a).

First look:

[...]

...

...

...

+/** + * Convert an ISF vector into an ISP vector + * + * @param[in] isf                  Isf vector + * @param[out] isp                 Output isp vector + * @param[in] size                 Isf/isp size + */ +static void isf2isp(const float *isf, double *isp, int size) +{ +    int i; + +    for (i = 0; i<  size - 1; i++) +        isp[i] = cos(2.0 * M_PI * isf[i]); + +    isp[size - 1] = cos(4.0 * M_PI * isf[size - 1]); +}

Almost duplication of amrnbdec.c:lsf2lsp().

Yes, but this last element doubling diverges between AMR-NB and AMR-WB reference sources.

Can't you just do "isp[size - 1] *=  2" or it is used elsewhere?

It's not the last element output that is doubled but the input. Since it is a cosine, I would have to calculate it again for the last element I guess.

[...]

...

...

...

+        for(j = i - 1; j>  1; j--) +            f[j] += f[j - 1] * val + f[j - 2]; +        f[1] += 0.25 * val; +    } +}

Hmm, can't this function be replaced by ff_lsp2polyf() with some rescaling?

Actually I realize now that the difference between these functions is just the entire output scaled down by 0.25. That was silly because I was scaling by 4.0 after this function. It is weird why the reference decoder does this process. Maybe it should worth someone checking it out to be sure about this.

I suppose you checked that after this change the output of your decoder didn't change, no?

Actually I cannot compare the output bitwise because the high band involves a random excitation, I am afraid that it is not the same for each run (or it is?). I did several comparisons manually between the resulting vectors using gdb and they were exactly the same. Also, I thought about it and I'm convinced that the code does the same as before. But it is weird, maybe someone should verify it.

...

...

...

+/** + * Convert a ISP vector to LP coefficient domain {a_k} + * Equations from TS 26.190 section 5.2.4 + * + * @param[in] isp                  ISP vector for a subframe + * @param[out] lp                  LP coefficients + * @param[in] lp_half_order        Half the number of LPs to construct + */ +static void isp2lp(const double *isp, float *lp, int lp_half_order) { +    double pa[10 + 1], qa[10 + 1]; +    double last_isp = isp[2 * lp_half_order - 1]; +    double qa_old = 0.0; +    float *lp2 =&lp[2 * lp_half_order]; +    int i; + +    if (lp_half_order>  8) { // high-band specific +        lsp2polyf_16k(isp,     pa, lp_half_order); +        lsp2polyf_16k(isp + 1, qa, lp_half_order - 1); + +        for (i = 0; i<= lp_half_order; i++) +            pa[i] *= 4.0; +        for (i = 0; i<  lp_half_order; i++) +            qa[i] *= 4.0; +    } else { +        ff_lsp2polyf(isp,     pa, lp_half_order); +        ff_lsp2polyf(isp + 1, qa, lp_half_order - 1); +    } + +    for (i = 1; i<  lp_half_order; i++) { +        double paf = (1 + last_isp) * pa[i]; +        double qaf = (1 - last_isp) * (qa[i] - qa_old); + +        qa_old = qa[i - 1]; + +        lp[i]   = 0.5 * (paf + qaf); +        lp2[-i] = 0.5 * (paf - qaf); +    } + +    lp[0] = 1.0; +    lp[lp_half_order] = 0.5 * (1 + last_isp) * pa[lp_half_order]; +    lp2[0] = last_isp; +}

Please double-check if this is not a duplication of sipr.c:lsp2lpc_sipr().

Interesting, I believe they are the same function except for the fact that in AMR-WB the first LP coefficient is always 1.0 but this should be easily adapted by passing lp + 1 to the function and setting lp[0] = 1.0. Also, the LP order is different between these codecs. To reuse this existing function it should be moved to another file (lsp.c I guess) and modified. Should I do it and include in the next patch?

You can send directly a patch to ffmpeg-devel to move this code to lsp.c.

Ok, I will try it. [...]

...

...

...

+/** + * Upsample a signal by 5/4 ratio (from 12.8kHz to 16kHz) using + * a FIR interpolation filter. Uses past data from before *in address + * + * @param[out] out                 Buffer for interpolated signal + * @param[in] in                   Current signal data (length 0.8*o_size) + * @param[in] o_size               Output signal length + */ +static void upsample_5_4(float *out, const float *in, int o_size) +{ +    const float *in0 = in - UPS_FIR_SIZE + 1; +    int i; + +    for (i = 0; i<  o_size; i++) { +        int int_part  = (4 * i) / 5; +        int frac_part = (4 * i) - 5 * int_part;

You can break this loop in two to avoid the division:

i = 0; for (j = 0; j<  o_size/5; j++)    for (k = 0; k<  5; k++) {        ....        i++;    }

I've done this in not so neat way, adding 4 and calculating a simple remainder by 5. Tell me if can be done in a better way.

Does something like the following work?

   int int_part = 0, frac_part = 0;

   i = 0;    for (j = 0; j<  o_size / 5; j++) {        out[i] = in[int_part];        frac_part = 4;        i++;        for (k = 1; k<  5; k++) {            out[i] = ff_dot_productf(in0 + int_part, upsample_fir[4 - frac_part],                                         UPS_MEM_SIZE);            int_part++;            frac_part--;            i++;       }    }

Yes, I guess it works. Despite being very specific to the 5/4 case, it should be more efficient. Fixed [...]

...

+/** + * Parses a speech frame, storing data in the Context + * + * @param[in,out] ctx              The context + * @param[in] buf                  Pointer to the input buffer + * @param[in] buf_size             Size of the input buffer + * + * @return The frame mode + */ +static enum Mode unpack_bitstream(AMRWBContext *ctx, const uint8_t *buf, +                                  int buf_size) +{ +    GetBitContext gb; +    uint16_t *data; + +    init_get_bits(&gb, buf, buf_size * 8); + +    /* decode frame header (1st octet) */ +    skip_bits(&gb, 1);  // padding bit +    ctx->fr_cur_mode  = get_bits(&gb, 4); +    ctx->fr_quality   = get_bits1(&gb); +    skip_bits(&gb, 2);  // padding bits + +    // XXX: We are using only the "MIME/storage" format +    // used by libopencore. This format is simpler and +    // does not carry the auxiliary information of the frame + +    data = (uint16_t *)&ctx->frame; +    memset(data, 0, sizeof(AMRWBFrame)); +    buf++; + +    if (ctx->fr_cur_mode<  MODE_SID) { /* Normal speech frame */ +        const uint16_t *perm = amr_bit_orderings_by_mode[ctx->fr_cur_mode]; +        int field_size; + +        while ((field_size = *perm++)) { +            int field = 0; +            int field_offset = *perm++; +            while (field_size--) { +               uint16_t bit_idx = *perm++; +               field<<= 1; +               /* The bit index inside the byte is reversed (MSB->LSB) */ +               field |= BIT_POS(buf[bit_idx>>  3], 7 - (bit_idx&  7)); +            } +            data[field_offset] = field; +        } +    } + +    return ctx->fr_cur_mode; +}

Can't you reorder the bit indexes in a way it is in the LSB->MSB so you can use just the usual bitstream functions?

Which bitstream functions? If the tables were in LSB->MSB order, it would be the same algorithm as in AMR-NB, the only difference is that I do (7 - index) to reverse the order. On 30 August 2010 10:00, Rob <robert.swain@gmail.com> wrote:

On 30 August 2010 14:31, Vitor Sessak <vitor1001@gmail.com> wrote:

...

On 08/27/2010 11:13 PM, Marcelo Galvão Póvoa wrote:

...

On 26 August 2010 18:55, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

Marcelo Galvão Póvoa wrote:

[...]

...

...

...

...

+/** + * Generate the high-band excitation with the same energy from the lower + * one and scaled by the given gain + * + * @param[in] ctx                  The context + * @param[out] hb_exc              Buffer for the excitation + * @param[in] synth_exc            Low-band excitation used for synthesis + * @param[in] hb_gain              Wanted excitation gain + */ +static void scaled_hb_excitation(AMRWBContext *ctx, float *hb_exc, +                                 const float *synth_exc, float hb_gain) +{ +    int i; +    float energy = ff_dot_productf(synth_exc, synth_exc, AMRWB_SFR_SIZE); + +    /* Generate a white-noise excitation */ +    for (i = 0; i<  AMRWB_SFR_SIZE_16k; i++) +        hb_exc[i] = 32768.0 - (uint16_t) av_lfg_get(&ctx->prng); + +    ff_scale_vector_to_given_sum_of_squares(hb_exc, hb_exc, energy, +                                            AMRWB_SFR_SIZE_16k); + +    for (i = 0; i<  AMRWB_SFR_SIZE_16k; i++) +        hb_exc[i] *= hb_gain; +}

Why are you scaling it twice?

The first scaling is to match the energy with the lower band part. The second is the high_band gain itself. This can be done in only one loop using ff_scale_vector_to_given_sum_of_squares() ?

I think you will be obliged to duplicate a little of  ff_scale_vector_to_given_sum_of_squares(), but it will make your code faster...

The point is to avoid iterating over the subframe to multiply by some value twice by merging the two beforehand and then applying to the vector. Looking at the code:

void ff_scale_vector_to_given_sum_of_squares(float *out, const float *in,                                             float sum_of_squares, const int n) {    int i;    float scalefactor = ff_dot_productf(in, in, n);    if (scalefactor)        scalefactor = sqrt(sum_of_squares / scalefactor);    for (i = 0; i < n; i++)        out[i] = in[i] * scalefactor; }

We want: scalefactor * hb_gain = sqrt(sum_of_squares / ff_dot_productf(in, in, n)) * hb_gain = sqrt(sum_of_squares * hb_gain * hb_gain / ff_dot_productf()) hb_gain is [0.1, 1.0] according to find_hb_gain() so there's no sign issue

So you should just be able to pass in energy * hb_gain * hb_gain I think. Or duplicate the code.

Yes, in fact it is better to pass energy * hb_gain * hb_gain. Fixed -- Marcelo

Sorry about the delay to reply On 30 August 2010 12:39, Vitor Sessak <vitor1001@gmail.com> wrote:

On 08/30/2010 04:44 PM, Marcelo Galvão Póvoa wrote:

...

On 30 August 2010 09:31, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 08/27/2010 11:13 PM, Marcelo Galvão Póvoa wrote:

...

On 26 August 2010 18:55, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

Marcelo Galvão Póvoa wrote:

...

Fixed excitation array incorrect length.

Should now be applicable cleanly to the latest ffmpeg revision (fd151a5f8bd152c456a).

First look:

[...]

...

...

...

+/** + * Convert an ISF vector into an ISP vector + * + * @param[in] isf                  Isf vector + * @param[out] isp                 Output isp vector + * @param[in] size                 Isf/isp size + */ +static void isf2isp(const float *isf, double *isp, int size) +{ +    int i; + +    for (i = 0; i<    size - 1; i++) +        isp[i] = cos(2.0 * M_PI * isf[i]); + +    isp[size - 1] = cos(4.0 * M_PI * isf[size - 1]); +}

Almost duplication of amrnbdec.c:lsf2lsp().

Yes, but this last element doubling diverges between AMR-NB and AMR-WB reference sources.

Can't you just do "isp[size - 1] *=  2" or it is used elsewhere?

It's not the last element output that is doubled but the input. Since it is a cosine, I would have to calculate it again for the last element I guess.

I mean doubling the last element of the input before calling the function, if it doesn't make the rest of the code more complex.

Well, I only call this function in two places, so it seems to worth reusing this way

...

...

[...]

...

...

...

+        for(j = i - 1; j>    1; j--) +            f[j] += f[j - 1] * val + f[j - 2]; +        f[1] += 0.25 * val; +    } +}

Hmm, can't this function be replaced by ff_lsp2polyf() with some rescaling?

Actually I realize now that the difference between these functions is just the entire output scaled down by 0.25. That was silly because I was scaling by 4.0 after this function. It is weird why the reference decoder does this process. Maybe it should worth someone checking it out to be sure about this.

I suppose you checked that after this change the output of your decoder didn't change, no?

Actually I cannot compare the output bitwise because the high band involves a random excitation, I am afraid that it is not the same for each run (or it is?).

Yes, you always send 1 as seed, so the prng output is always the same (and this is a good thing), but since you use floats output might not be bitexact. I suggest you do:

./tests/tiny_psnr output_ref.wav output_patched.wav 2

And see if stddev is ~ 0.1

By "output_ref.wav" you mean the earlier output from my decoder or the fixed-point reference implementation? I did both tests comparing my recent decoder with a 1kHz sine wave to: - 3GPP: stddev: 340.60 PSNR: 45.68 MAXDIFF: 1130 bytes: 959360/ 959360 - Mine before the reviews: stddev: 0.00 PSNR:999.99 MAXDIFF: 0 bytes: 959360/ 959360 I don't know how to analyze all these values

[...]

...

+/**

...

...

+ * Parses a speech frame, storing data in the Context + * + * @param[in,out] ctx              The context + * @param[in] buf                  Pointer to the input buffer + * @param[in] buf_size             Size of the input buffer + * + * @return The frame mode + */ +static enum Mode unpack_bitstream(AMRWBContext *ctx, const uint8_t *buf, +                                  int buf_size) +{ +    GetBitContext gb; +    uint16_t *data; + +    init_get_bits(&gb, buf, buf_size * 8); + +    /* decode frame header (1st octet) */ +    skip_bits(&gb, 1);  // padding bit +    ctx->fr_cur_mode  = get_bits(&gb, 4); +    ctx->fr_quality   = get_bits1(&gb); +    skip_bits(&gb, 2);  // padding bits + +    // XXX: We are using only the "MIME/storage" format +    // used by libopencore. This format is simpler and +    // does not carry the auxiliary information of the frame + +    data = (uint16_t *)&ctx->frame; +    memset(data, 0, sizeof(AMRWBFrame)); +    buf++; + +    if (ctx->fr_cur_mode<    MODE_SID) { /* Normal speech frame */ +        const uint16_t *perm = amr_bit_orderings_by_mode[ctx->fr_cur_mode]; +        int field_size; + +        while ((field_size = *perm++)) { +            int field = 0; +            int field_offset = *perm++; +            while (field_size--) { +               uint16_t bit_idx = *perm++; +               field<<= 1; +               /* The bit index inside the byte is reversed (MSB->LSB) */ +               field |= BIT_POS(buf[bit_idx>>    3], 7 - (bit_idx&  7)); +            } +            data[field_offset] = field; +        } +    } + +    return ctx->fr_cur_mode; +}

Can't you reorder the bit indexes in a way it is in the LSB->MSB so you can use just the usual bitstream functions?

Which bitstream functions?

Those in libavcodec/get_bits.h (get_bits(), etc).

I am following the unpack_bitstream() from AMR-NB

...

If the tables were in LSB->MSB order, it would be the same algorithm as in AMR-NB, the only difference is that I do (7 - index) to reverse the order.

Can't you change the tables in a way you can reuse the unpack function of AMRNB?

I guess so, after all it's an interesting method of reordering and assigning the variables So, until now we have three functions that can be reused and would need to move their code to an appropriate header file: - sipr.c:lsp2lpc_sipr() -> lsp.h: Do you have a suggestion to a new name for this? There is already a function called ff_acelp_lsp2lpc() - amrnbdec.c:lsf2lsp() -> lsp.h - internal part of amrnbdec.c:unpack_bitstream() -> get_bits.h (I guess) I will try to create patches for these modifications and send to ffmpeg-devel -- Marcelo

On 08/30/2010 02:31 PM, Vitor Sessak wrote:

On 08/27/2010 11:13 PM, Marcelo Galvão Póvoa wrote:

...

On 26 August 2010 18:55, Vitor Sessak<vitor1001@gmail.com> wrote:

...

Marcelo Galvão Póvoa wrote:

[...]

...

...

...

+/** + * Apply to synthesis a 2nd order high-pass filter + * + * @param[out] out Buffer for filtered output + * @param[in] hpf_coef Filter coefficients as used below + * @param[in,out] mem State from last filtering (updated) + * @param[in] in Input speech data + * + * @remark It is safe to pass the same array in in and out parameters + */ +static void high_pass_filter(float *out, const float hpf_coef[2][3], + float mem[4], const float *in) +{ + int i; + float *x = mem - 1, *y = mem + 2; // previous inputs and outputs + + for (i = 0; i< AMRWB_SFR_SIZE; i++) { + float x0 = in[i]; + + out[i] = hpf_coef[0][0] * x0 + hpf_coef[1][0] * y[0] + + hpf_coef[0][1] * x[1] + hpf_coef[1][1] * y[1] + + hpf_coef[0][2] * x[2]; + + y[1] = y[0]; + y[0] = out[i]; + + x[2] = x[1]; + x[1] = x0; + } +} acelp_filter.c:ff_acelp_apply_order_2_transfer_function()

Are you sure? I can't see them as equivalent, could you explain?

I'll give a better look at it soon...

Try the attached patch, it practically does not change the output: deus-23k85.awbstddev: 0.08 PSNR:118.01 MAXDIFF: 1 bytes: 1054720/ 1054720 seed-12k65.awbstddev: 0.06 PSNR:119.94 MAXDIFF: 1 bytes: 327680/ 327680 seed-14k25.awbstddev: 0.06 PSNR:119.59 MAXDIFF: 1 bytes: 327680/ 327680 seed-15k85.awbstddev: 0.06 PSNR:119.92 MAXDIFF: 1 bytes: 327680/ 327680 seed-18k25.awbstddev: 0.07 PSNR:119.32 MAXDIFF: 1 bytes: 327680/ 327680 seed-19k85.awbstddev: 0.07 PSNR:119.20 MAXDIFF: 1 bytes: 327680/ 327680 seed-23k05.awbstddev: 0.07 PSNR:119.35 MAXDIFF: 1 bytes: 327680/ 327680 seed-23k85.awbstddev: 0.07 PSNR:119.42 MAXDIFF: 1 bytes: 327680/ 327680 seed-6k60.awbstddev: 0.06 PSNR:119.61 MAXDIFF: 1 bytes: 327680/ 327680 seed-8k85.awbstddev: 0.07 PSNR:119.19 MAXDIFF: 1 bytes: 327680/ 327680 -Vitor

On 09/03/2010 11:50 PM, Marcelo Galvão Póvoa wrote:

On 3 September 2010 05:19, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 08/30/2010 02:31 PM, Vitor Sessak wrote:

...

On 08/27/2010 11:13 PM, Marcelo Galvão Póvoa wrote:

...

On 26 August 2010 18:55, Vitor Sessak<vitor1001@gmail.com> wrote:

...

Marcelo Galvão Póvoa wrote:

[...]

...

...

...

...

+/** + * Apply to synthesis a 2nd order high-pass filter + * + * @param[out] out Buffer for filtered output + * @param[in] hpf_coef Filter coefficients as used below + * @param[in,out] mem State from last filtering (updated) + * @param[in] in Input speech data + * + * @remark It is safe to pass the same array in in and out parameters + */ +static void high_pass_filter(float *out, const float hpf_coef[2][3], + float mem[4], const float *in) +{ + int i; + float *x = mem - 1, *y = mem + 2; // previous inputs and outputs + + for (i = 0; i< AMRWB_SFR_SIZE; i++) { + float x0 = in[i]; + + out[i] = hpf_coef[0][0] * x0 + hpf_coef[1][0] * y[0] + + hpf_coef[0][1] * x[1] + hpf_coef[1][1] * y[1] + + hpf_coef[0][2] * x[2]; + + y[1] = y[0]; + y[0] = out[i]; + + x[2] = x[1]; + x[1] = x0; + } +}

acelp_filter.c:ff_acelp_apply_order_2_transfer_function()

Are you sure? I can't see them as equivalent, could you explain?

I'll give a better look at it soon...

Try the attached patch, it practically does not change the output:

deus-23k85.awbstddev: 0.08 PSNR:118.01 MAXDIFF: 1 bytes: 1054720/ 1054720 seed-12k65.awbstddev: 0.06 PSNR:119.94 MAXDIFF: 1 bytes: 327680/ 327680 seed-14k25.awbstddev: 0.06 PSNR:119.59 MAXDIFF: 1 bytes: 327680/ 327680 seed-15k85.awbstddev: 0.06 PSNR:119.92 MAXDIFF: 1 bytes: 327680/ 327680 seed-18k25.awbstddev: 0.07 PSNR:119.32 MAXDIFF: 1 bytes: 327680/ 327680 seed-19k85.awbstddev: 0.07 PSNR:119.20 MAXDIFF: 1 bytes: 327680/ 327680 seed-23k05.awbstddev: 0.07 PSNR:119.35 MAXDIFF: 1 bytes: 327680/ 327680 seed-23k85.awbstddev: 0.07 PSNR:119.42 MAXDIFF: 1 bytes: 327680/ 327680 seed-6k60.awbstddev: 0.06 PSNR:119.61 MAXDIFF: 1 bytes: 327680/ 327680 seed-8k85.awbstddev: 0.07 PSNR:119.19 MAXDIFF: 1 bytes: 327680/ 327680

I've tried testing with two other samples I encoded and the results were: all_man.awb stddev: 0.24 PSNR:108.64 MAXDIFF: 1 bytes: 473600/ 473600 conference-23k85.awb stddev: 0.20 PSNR:109.95 MAXDIFF: 1 bytes: 157440/ 157440

I think I should apply this patch then?

Yes, 0.24 is a very small difference.

I noticed that when I run the comparison for deus-23k85.awb the stddev is 0.16, not 0.08 like yours. Maybe it's because I am on OS X or I did something wrong?

It happens. Floating point output might vary depending if on your CPU and your compiler.

Also, whether with this patch or not I guess my output to the first sample above (attached) is not good compared to the reference decoder. The are some sharp peaks that can be seen with Audacity, one of them also with clipping. The stddev I've got seems ugly: stddev: 884.86 PSNR: 37.39 MAXDIFF:23298 bytes: 473600/ 473600

Indeed, the stddev is not particularly good, but don't trust only it to do comparisons. A very small phase shift can completely ruin the stddev. Do you see sharp peaks in your output that are not there in the reference decoder? Or the sharp peaks shows up when you invert and superpose the waves? Do these sharp peaks gives audible artifacts?

Inverting one of the waves and playing them simultaneously I can hear some intermittent artifacts, so I guess there is still something wrong with the decoder.

This is also something that is very sensible to a tiny phase difference... -Vitor

On 09/04/2010 05:22 PM, Marcelo Galvão Póvoa wrote:

On 4 September 2010 06:02, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 09/03/2010 11:50 PM, Marcelo Galvão Póvoa wrote:

...

Also, whether with this patch or not I guess my output to the first sample above (attached) is not good compared to the reference decoder. The are some sharp peaks that can be seen with Audacity, one of them also with clipping. The stddev I've got seems ugly: stddev: 884.86 PSNR: 37.39 MAXDIFF:23298 bytes: 473600/ 473600

Indeed, the stddev is not particularly good, but don't trust only it to do comparisons. A very small phase shift can completely ruin the stddev.

Do you see sharp peaks in your output that are not there in the reference decoder? Or the sharp peaks shows up when you invert and superpose the waves? Do these sharp peaks gives audible artifacts?

Sorry, I've forgot to mention that I was comparing the reference decoder output waveform with mine. In general, they look mostly the same but there are some 3 or 4 points in which my decoder gives a short peak. I couldn't notice audible artifacts at these moments.

It does looks like a bug :( -Vitor

2010/9/4 Marcelo Galvão Póvoa <marspeoplester@gmail.com>:

On 4 September 2010 15:16, Vitor Sessak <vitor1001@gmail.com> wrote:

...

On 09/04/2010 05:22 PM, Marcelo Galvão Póvoa wrote:

...

On 4 September 2010 06:02, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/03/2010 11:50 PM, Marcelo Galvão Póvoa wrote:

...

Also, whether with this patch or not I guess my output to the first sample above (attached) is not good compared to the reference decoder. The are some sharp peaks that can be seen with Audacity, one of them also with clipping. The stddev I've got seems ugly: stddev:  884.86 PSNR: 37.39 MAXDIFF:23298 bytes:   473600/   473600

Indeed, the stddev is not particularly good, but don't trust only it to do comparisons. A very small phase shift can completely ruin the stddev.

Do you see sharp peaks in your output that are not there in the reference decoder? Or the sharp peaks shows up when you invert and superpose the waves? Do these sharp peaks gives audible artifacts?

Sorry, I've forgot to mention that I was comparing the reference decoder output waveform with mine. In general, they look mostly the same but there are some 3 or 4 points in which my decoder gives a short peak. I couldn't notice audible artifacts at these moments.

It does looks like a bug :(

I've just tested dumping all the voice_factor() values from the reference decoder (I suspected this function was problematic) and the output difference was not too much:

all_man.awb stddev:    0.38 PSNR:104.69 MAXDIFF:    4 bytes:   473600/   473600

The sharp peaks and the clipping are still there, so I guess we can discard checking this function.

I've forgot to mention that the high-band part isn't guilty too. Looking at the waveforms below it's strange because most of the time they look quite much the same but only at these few parts they start to diverge greatly. I'll try to investigate what happens just before the clipping (starting at +- 9.650 seconds) http://www.students.ic.unicamp.br/~ra082115/all_men_ref.wav http://www.students.ic.unicamp.br/~ra082115/am_vfOK.wav -- Marcelo

Ok, fortunately I've found the bug! It was just a MIN_ISF_SPACING parameter which I extracted from the reference code but was unsure about it's Q level. After some time, I thought I have it figured out but I was wrong. Now I know the answer the hard way... The clipping and the sharp peaks are gone, the waveforms are really close now! Also, the stddev against the reference decoder decreased a lot (it was ~884 before): all_men.awb stddev: 51.72 PSNR: 62.05 MAXDIFF: 1089 bytes: 473600/ 473600 So, here is how the decoder is now (I'm sending my commits to github also). It is still missing the bitstream_unpack() function sharing with AMR-NB which I will do soon. Is creating an amr.c file only for this really a feasible solution? Marcelo

On 09/06/2010 02:46 AM, Marcelo Galvão Póvoa wrote:

Ok, fortunately I've found the bug!

It was just a MIN_ISF_SPACING parameter which I extracted from the reference code but was unsure about it's Q level. After some time, I thought I have it figured out but I was wrong. Now I know the answer the hard way...

The clipping and the sharp peaks are gone, the waveforms are really close now!

That's great news!

Also, the stddev against the reference decoder decreased a lot (it was ~884 before): all_men.awb stddev: 51.72 PSNR: 62.05 MAXDIFF: 1089 bytes: 473600/ 473600

stddev of 51 looks pretty good to me for this case.

So, here is how the decoder is now (I'm sending my commits to github also).

It is still missing the bitstream_unpack() function sharing with AMR-NB which I will do soon.

I'll try to review some more your code as soon as I have some time. -Vitor

Hello, On 6 September 2010 10:13, Ronald S. Bultje <rsbultje@gmail.com> wrote:

Hi,

On Mon, Sep 6, 2010 at 5:54 AM, Vitor Sessak <vitor1001@gmail.com> wrote:

...

On 09/06/2010 02:46 AM, Marcelo Galvão Póvoa wrote:

...

Ok, fortunately I've found the bug!

It was just a MIN_ISF_SPACING parameter which I extracted from the reference code but was unsure about it's Q level. After some time, I thought I have it figured out but I was wrong. Now I know the answer the hard way...

The clipping and the sharp peaks are gone, the waveforms are really close now!

That's great news!

...

Also, the stddev against the reference decoder decreased a lot (it was ~884 before): all_men.awb stddev:   51.72 PSNR: 62.05 MAXDIFF: 1089 bytes:   473600/ 473600

stddev of 51 looks pretty good to me for this case.

Maxdiff of 1089 looks like a lot to me, with a low stddev that suggests that one particular part is off. Can you trace which part is off and why (phase shift vs. actual bug)?

Sorry, but can you suggest a way of doing it? Also, what do MAXDIFF and the "2" at the end of the command line mean? This sample have long silence parts and I'm comparing my floating point implementation to the reference 16-bit fixed point. How close you think they should be? I did the same test with the AMR-NB decoder from ffmpeg and the result was the following: stddev: 46.36 PSNR: 63.00 MAXDIFF: 452 bytes: 236800/ 236800 -- Marcelo

On 8 September 2010 06:54, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/07/2010 03:21 AM, Marcelo Galvão Póvoa wrote:

...

Hello,

On 6 September 2010 10:13, Ronald S. Bultje<rsbultje@gmail.com>  wrote:

...

Hi,

On Mon, Sep 6, 2010 at 5:54 AM, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/06/2010 02:46 AM, Marcelo Galvão Póvoa wrote:

...

Ok, fortunately I've found the bug!

It was just a MIN_ISF_SPACING parameter which I extracted from the reference code but was unsure about it's Q level. After some time, I thought I have it figured out but I was wrong. Now I know the answer the hard way...

The clipping and the sharp peaks are gone, the waveforms are really close now!

That's great news!

...

Also, the stddev against the reference decoder decreased a lot (it was ~884 before): all_men.awb stddev:   51.72 PSNR: 62.05 MAXDIFF: 1089 bytes:   473600/ 473600

stddev of 51 looks pretty good to me for this case.

Maxdiff of 1089 looks like a lot to me, with a low stddev that suggests that one particular part is off. Can you trace which part is off and why (phase shift vs. actual bug)?

Sorry, but can you suggest a way of doing it?

Your method of inverting one sample and summing in audacity would work on showing where it is happening (some point will have an amplitude of 1089). To know if is a phase shift or a bug, you will have to compare visually both waves.

I don't know exactly how to detect a phase shift this way but the difference waveform I obtained [1] has some peaks at the sibilant parts I think. Probably just where the high band is louder.

...

Also, what do MAXDIFF and the "2" at the end of the command line mean?

MAXDIFF is the biggest difference among two samples. The "2" at the end of the command line says to read two-byte integers (16-bit). If you were comparing video pixels, you would use "1". You can also see the source of tiny_psnr.c, it is pretty simple.

...

This sample have long silence parts and I'm comparing my floating point implementation to the reference 16-bit fixed point. How close you think they should be?

A very small stddev (< 1.00) would assure there is no bug in your decoder, but the fact that it is large does not means there is one.

I suggest you do the following test:

a) Get a biggish file (> 30 minutes) b) Convert it to the a WAV with the sample rate and number of channel the AMR encoder takes as input c) Encode the file obtained in (b) it with the reference encoder d) Decode the file obtained in (c) with the reference decoder e) Decode the file obtained in (c) with ffamrwb f) Compare the stddev of files obtained in (b) and (d) with that of (b) and (e). If file decoded with ffamr are as close to the original as that decoded with the reference decoder, it's good.

Results: $ ./tests/tiny_psnr ~/ref_pod.wav ~/orig_pod.wav 2 stddev: 2599.69 PSNR: 28.03 MAXDIFF:39592 bytes: 76480640/ 76480660 $ ./tests/tiny_psnr ~/my_pod.wav ~/orig_pod.wav 2 stddev: 2600.02 PSNR: 28.03 MAXDIFF:39653 bytes: 76480640/ 76480660 $ ./tests/tiny_psnr ~/my_pod.wav ~/ref_pod.wav 2 stddev: 95.62 PSNR: 56.72 MAXDIFF: 5529 bytes: 76480640/ 76480640 By removing the fractional part of the excitation as AMR-NB does, the result was slightly better: $ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 stddev: 2589.73 PSNR: 28.06 MAXDIFF:39639 bytes: 76480640/ 76480660 I think these are good news! It was much closer than I would expect. The test file was a 39:50 long podcast transcoded from mp3. [1] http://www.students.ic.unicamp.br/~ra082115/all_men_diff.wav -- Marcelo

On 8 September 2010 23:10, Ronald S. Bultje <rsbultje@gmail.com> wrote:

Hi,

2010/9/8 Marcelo Galvão Póvoa <marspeoplester@gmail.com>:

...

$ ./tests/tiny_psnr ~/my_pod.wav ~/ref_pod.wav 2 stddev:   95.62 PSNR: 56.72 MAXDIFF: 5529 bytes: 76480640/ 76480640

By removing the fractional part of the excitation as AMR-NB does, the result was slightly better: $ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 stddev: 2589.73 PSNR: 28.06 MAXDIFF:39639 bytes: 76480640/ 76480660

That isnt better, much worse.

You are comparing different kind of tests I did. This 95.62 came from the comparison between the output of my decoder and the reference decoder. The comparison against the original source as suggested by Vitor are those lines with "orig_pod.wav" in it. The stddev came down from 2600.02 to 2589.73 -- Marcelo

On 9 September 2010 05:11, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/09/2010 02:50 AM, Marcelo Galvão Póvoa wrote:

...

On 8 September 2010 06:54, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/07/2010 03:21 AM, Marcelo Galvão Póvoa wrote:

...

Hello,

On 6 September 2010 10:13, Ronald S. Bultje<rsbultje@gmail.com>  wrote:

...

Hi,

On Mon, Sep 6, 2010 at 5:54 AM, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/06/2010 02:46 AM, Marcelo Galvão Póvoa wrote: > > Ok, fortunately I've found the bug! > > It was just a MIN_ISF_SPACING parameter which I extracted from the > reference code but was unsure about it's Q level. After some time, I > thought I have it figured out but I was wrong. Now I know the answer > the hard way... > > The clipping and the sharp peaks are gone, the waveforms are really > close now!

That's great news!

> Also, the stddev against the reference decoder decreased a > lot (it was ~884 before): > all_men.awb stddev:   51.72 PSNR: 62.05 MAXDIFF: 1089 bytes: > 473600/ > 473600

stddev of 51 looks pretty good to me for this case.

Maxdiff of 1089 looks like a lot to me, with a low stddev that suggests that one particular part is off. Can you trace which part is off and why (phase shift vs. actual bug)?

Sorry, but can you suggest a way of doing it?

Your method of inverting one sample and summing in audacity would work on showing where it is happening (some point will have an amplitude of 1089). To know if is a phase shift or a bug, you will have to compare visually both waves.

I don't know exactly how to detect a phase shift this way but the difference waveform I obtained [1] has some peaks at the sibilant parts I think. Probably just where the high band is louder.

...

...

Also, what do MAXDIFF and the "2" at the end of the command line mean?

MAXDIFF is the biggest difference among two samples. The "2" at the end of the command line says to read two-byte integers (16-bit). If you were comparing video pixels, you would use "1". You can also see the source of tiny_psnr.c, it is pretty simple.

...

This sample have long silence parts and I'm comparing my floating point implementation to the reference 16-bit fixed point. How close you think they should be?

A very small stddev (<  1.00) would assure there is no bug in your decoder, but the fact that it is large does not means there is one.

I suggest you do the following test:

a) Get a biggish file (>  30 minutes) b) Convert it to the a WAV with the sample rate and number of channel the AMR encoder takes as input c) Encode the file obtained in (b) it with the reference encoder d) Decode the file obtained in (c) with the reference decoder e) Decode the file obtained in (c) with ffamrwb f) Compare the stddev of files obtained in (b) and (d) with that of (b) and (e). If file decoded with ffamr are as close to the original as that decoded with the reference decoder, it's good.

Results: $ ./tests/tiny_psnr ~/ref_pod.wav ~/orig_pod.wav 2 stddev: 2599.69 PSNR: 28.03 MAXDIFF:39592 bytes: 76480640/ 76480660 $ ./tests/tiny_psnr ~/my_pod.wav ~/orig_pod.wav 2 stddev: 2600.02 PSNR: 28.03 MAXDIFF:39653 bytes: 76480640/ 76480660 $ ./tests/tiny_psnr ~/my_pod.wav ~/ref_pod.wav 2

Hmm, the files have different sizes. Are you sure you are not comparing files shifted of a few bytes? One parameter of tiny_psnr is a shift between the two files. Does

tiny_psnr ~/ref_pod.wav ~/orig_pod.wav 2 10

Gives a much worse result? If the shift you are using now (zero) is correct, changing it to anything else should make stddev much bigger. Can you also put the wav files somewhere I can download? If you have a problem of quota, you can use our FTP server as explained in [1].

Now that seems weird: $ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 10 stddev: 2575.45 PSNR: 28.11 MAXDIFF:46172 bytes: 76480630/ 76480660 $ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 -20 stddev: 2524.06 PSNR: 28.29 MAXDIFF:45328 bytes: 76480640/ 76480640 $ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 200 stddev: 2090.99 PSNR: 29.92 MAXDIFF:46167 bytes: 76480440/ 76480660 I will upload the files later. Probably it will take some time. -- Marcelo

On 09/09/2010 07:42 PM, Marcelo Galvão Póvoa wrote:

2010/9/9 Marcelo Galvão Póvoa<marspeoplester@gmail.com>:

...

On 9 September 2010 05:11, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 09/09/2010 02:50 AM, Marcelo Galvão Póvoa wrote:

...

On 8 September 2010 06:54, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 09/07/2010 03:21 AM, Marcelo Galvão Póvoa wrote:

...

Hello,

On 6 September 2010 10:13, Ronald S. Bultje<rsbultje@gmail.com> wrote: > > Hi, > > On Mon, Sep 6, 2010 at 5:54 AM, Vitor Sessak<vitor1001@gmail.com> > wrote: >> >> On 09/06/2010 02:46 AM, Marcelo Galvão Póvoa wrote: >>> >>> Ok, fortunately I've found the bug! >>> >>> It was just a MIN_ISF_SPACING parameter which I extracted from the >>> reference code but was unsure about it's Q level. After some time, I >>> thought I have it figured out but I was wrong. Now I know the answer >>> the hard way... >>> >>> The clipping and the sharp peaks are gone, the waveforms are really >>> close now! >> >> That's great news! >> >>> Also, the stddev against the reference decoder decreased a >>> lot (it was ~884 before): >>> all_men.awb stddev: 51.72 PSNR: 62.05 MAXDIFF: 1089 bytes: >>> 473600/ >>> 473600 >> >> stddev of 51 looks pretty good to me for this case. > > Maxdiff of 1089 looks like a lot to me, with a low stddev that > suggests that one particular part is off. Can you trace which part is > off and why (phase shift vs. actual bug)? >

Sorry, but can you suggest a way of doing it?

Your method of inverting one sample and summing in audacity would work on showing where it is happening (some point will have an amplitude of 1089). To know if is a phase shift or a bug, you will have to compare visually both waves.

I don't know exactly how to detect a phase shift this way but the difference waveform I obtained [1] has some peaks at the sibilant parts I think. Probably just where the high band is louder.

...

...

Also, what do MAXDIFF and the "2" at the end of the command line mean?

MAXDIFF is the biggest difference among two samples. The "2" at the end of the command line says to read two-byte integers (16-bit). If you were comparing video pixels, you would use "1". You can also see the source of tiny_psnr.c, it is pretty simple.

...

This sample have long silence parts and I'm comparing my floating point implementation to the reference 16-bit fixed point. How close you think they should be?

A very small stddev (< 1.00) would assure there is no bug in your decoder, but the fact that it is large does not means there is one.

I suggest you do the following test:

a) Get a biggish file (> 30 minutes) b) Convert it to the a WAV with the sample rate and number of channel the AMR encoder takes as input c) Encode the file obtained in (b) it with the reference encoder d) Decode the file obtained in (c) with the reference decoder e) Decode the file obtained in (c) with ffamrwb f) Compare the stddev of files obtained in (b) and (d) with that of (b) and (e). If file decoded with ffamr are as close to the original as that decoded with the reference decoder, it's good.

Results: $ ./tests/tiny_psnr ~/ref_pod.wav ~/orig_pod.wav 2 stddev: 2599.69 PSNR: 28.03 MAXDIFF:39592 bytes: 76480640/ 76480660 $ ./tests/tiny_psnr ~/my_pod.wav ~/orig_pod.wav 2 stddev: 2600.02 PSNR: 28.03 MAXDIFF:39653 bytes: 76480640/ 76480660 $ ./tests/tiny_psnr ~/my_pod.wav ~/ref_pod.wav 2

Hmm, the files have different sizes. Are you sure you are not comparing files shifted of a few bytes? One parameter of tiny_psnr is a shift between the two files. Does

tiny_psnr ~/ref_pod.wav ~/orig_pod.wav 2 10

Gives a much worse result? If the shift you are using now (zero) is correct, changing it to anything else should make stddev much bigger. Can you also put the wav files somewhere I can download? If you have a problem of quota, you can use our FTP server as explained in [1].

Now that seems weird:

$ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 10 stddev: 2575.45 PSNR: 28.11 MAXDIFF:46172 bytes: 76480630/ 76480660

$ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 -20 stddev: 2524.06 PSNR: 28.29 MAXDIFF:45328 bytes: 76480640/ 76480640

$ ./tests/tiny_psnr ~/my_pod2.wav ~/orig_pod.wav 2 200 stddev: 2090.99 PSNR: 29.92 MAXDIFF:46167 bytes: 76480440/ 76480660

I will upload the files later. Probably it will take some time.

Ok, I've put them in /MPlayer/incoming/AMR-WB

Thanks for the files. I get vitor@vitor:~$ tiny_psnr orig_pod.wav ref_pod.wav 2 -190 stddev: 648.78 PSNR: 40.09 MAXDIFF:10660 bytes: 76480660/ 76480450 vitor@vitor:~$ tiny_psnr orig_pod.wav soc_pod.wav 2 -190 stddev: 643.05 PSNR: 40.16 MAXDIFF:10705 bytes: 76480660/ 76480450 which does settle the quality issue for me :D -Vitor

On 6 September 2010 11:31, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote:

...

Ok, fortunately I've found the bug!

[...]

...

So, here is how the decoder is now (I'm sending my commits to github also).

...

+/** Get x bits in the index interval [lsb,lsb+len-1] inclusive */ +#define BIT_STR(x,lsb,len) (((x)>>  (lsb))&  ((1<<  (len)) - 1)) + +/** Get the bit at specified position */ +#define BIT_POS(x, p) (((x)>>  (p))&  1)

If you move the #defines closer to where it is used, you improve the readability of your code.

Fixed

...

+ +/** + * Decodes quantized ISF vectors using 36-bit indices (6K60 mode only) + * + * @param[in] ind                  Array of 5 indices + * @param[out] isf_q               Buffer for isf_q[LP_ORDER] + * + */ +static void decode_isf_indices_36b(uint16_t *ind, float *isf_q) { +    int i; + +    for (i = 0; i<  9; i++) { +        isf_q[i] = dico1_isf[ind[0]][i] * (1.0f / (1<<  15)); +    } +    for (i = 0; i<  7; i++) { +        isf_q[i + 9] = dico2_isf[ind[1]][i] * (1.0f / (1<<  15)); +    } +    for (i = 0; i<  5; i++) { +        isf_q[i] += dico21_isf_36b[ind[2]][i] * (1.0f / (1<<  15)); +    } +    for (i = 0; i<  4; i++) { +        isf_q[i + 5] += dico22_isf_36b[ind[3]][i] * (1.0f / (1<<  15)); +    } +    for (i = 0; i<  7; i++) { +        isf_q[i + 9] += dico23_isf_36b[ind[4]][i] * (1.0f / (1<<  15)); +    } +}

I think this would be more readable if the (1.0f / (1 << 15)) rescaling would be done in isf_add_mean_and_past().

Hmm, it improves readability at this part but I think that way the output for decode_isf_indices_36b() and the input for isf_add_mean_and_past() would have a strange meaning, if you know what I mean.

...

+ +/** + * Find the pitch vector by interpolating the past excitation at the + * pitch delay, which is obtained in this function + * + * @param[in,out] ctx              The context + * @param[in] amr_subframe         Current subframe data + * @param[in] subframe             Current subframe index (0 to 3) + */ +static void decode_pitch_vector(AMRWBContext *ctx, +                                const AMRWBSubFrame *amr_subframe, +                                const int subframe) +{ +    int pitch_lag_int, pitch_lag_frac; +    int i; +    float *exc     = ctx->excitation; +    enum Mode mode = ctx->fr_cur_mode; + +    if (mode<= MODE_8k85) { +        decode_pitch_lag_low(&pitch_lag_int,&pitch_lag_frac, amr_subframe->adap, +&ctx->base_pitch_lag, subframe, mode); +    } else +        decode_pitch_lag_high(&pitch_lag_int,&pitch_lag_frac, amr_subframe->adap, +&ctx->base_pitch_lag, subframe); + +    ctx->pitch_lag_int = pitch_lag_int;

...

+    pitch_lag_int     += (pitch_lag_frac<  0 ? -1 : 0) + (pitch_lag_frac ? 1 : 0);

pitch_lag_int     += pitch_lag_frac ? FFSIGN(pitch_lag_frac) : 0;

I don't see if I understand this correctly, see my "truth table" below: pitch_lag_frac | mine_RHS | your_RHS <0 | 0 | -1 =0 | 0 | 0

0 | 1 | 1

So, the simplified form would be this? pitch_lag_int += pitch_lag_frac > 0 ? 1 : 0;

...

+/** + * Reduce fixed vector sparseness by smoothing with one of three IR filters + * Also known as "adaptive phase dispersion" + * + * @param[in] ctx                  The context + * @param[in,out] fixed_vector     Unfiltered fixed vector + * @param[out] buf                 Space for modified vector if necessary + * + * @return The potentially overwritten filtered fixed vector address + */ +static float *anti_sparseness(AMRWBContext *ctx, +                              float *fixed_vector, float *buf) +{ +    int ir_filter_nr; + +    if (ctx->fr_cur_mode>  MODE_8k85) // no filtering in higher modes +        return fixed_vector; + +    if (ctx->pitch_gain[0]<  0.6) { +        ir_filter_nr = 0;      // strong filtering +    } else if (ctx->pitch_gain[0]<  0.9) { +        ir_filter_nr = 1;      // medium filtering +    } else +        ir_filter_nr = 2;      // no filtering + +    /* detect 'onset' */ +    if (ctx->fixed_gain[0]>  3.0 * ctx->fixed_gain[1]) { +        if (ir_filter_nr<  2) +            ir_filter_nr++; +    } else { +        int i, count = 0; + +        for (i = 0; i<  6; i++) +            if (ctx->pitch_gain[i]<  0.6) +                count++; + +        if (count>  2) +            ir_filter_nr = 0; + +        if (ir_filter_nr>  ctx->prev_ir_filter_nr + 1) +            ir_filter_nr--; +    } + +    /* update ir filter strength history */ +    ctx->prev_ir_filter_nr = ir_filter_nr; + +    ir_filter_nr += (ctx->fr_cur_mode == MODE_8k85 ? 1 : 0); + +    if (ir_filter_nr<  2) { +        int i, j; +        const float *coef = ir_filters_lookup[ir_filter_nr]; + +        /* Circular convolution code in the reference +         * decoder was modified to avoid using one +         * extra array. The filtered vector is given by: +         * +         * c2(n) = sum(i,0,len-1){ c(i) * coef( (n - i + len) % len ) } +         */ + +        memset(buf, 0, sizeof(float) * AMRWB_SFR_SIZE); +        for (i = 0; i<  AMRWB_SFR_SIZE; i++)

...

+            if (fixed_vector[i]) { +                int li = AMRWB_SFR_SIZE - i; + +                for (j = 0; j<  li; j++) +                    buf[i + j] += fixed_vector[i] * coef[j]; + +                for (j = 0; j<  i; j++) +                    buf[j] += fixed_vector[i] * coef[j + li]; +            }

Can celp_filters.c:ff_celp_circ_addf() simplify this?

I already gave some thought to that but I couldn't figure out how to use ff_celp_circ_addf() there. I wrote the algorithm the simplest way I could think of.

...

+/** + * Extrapolate a ISF vector to the 16kHz range (20th order LP) + * used at mode 6k60 LP filter for the high frequency band + * + * @param[out] out                 Buffer for extrapolated isf + * @param[in] isf                  Input isf vector + */ +static void extrapolate_isf(float out[LP_ORDER_16k], float isf[LP_ORDER]) +{ +    float diff_isf[LP_ORDER - 2], diff_mean; +    float *diff_hi = diff_isf - LP_ORDER + 1; // diff array for extrapolated indices +    float corr_lag[3]; +    float est, scale; +    int i, i_max_corr; + +    memcpy(out, isf, (LP_ORDER - 1) * sizeof(float)); +    out[LP_ORDER_16k - 1] = isf[LP_ORDER - 1]; + +    /* Calculate the difference vector */ +    for (i = 0; i<  LP_ORDER - 2; i++) +        diff_isf[i] = isf[i + 1] - isf[i]; + +    diff_mean = 0.0; +    for (i = 2; i<  LP_ORDER - 2; i++) +        diff_mean += diff_isf[i] / (LP_ORDER - 4);

diff_mean += diff_isf[i] * (1.0f / (LP_ORDER - 4));

Fixed

...

+    for (i = LP_ORDER - 1; i<  LP_ORDER_16k - 1; i++) +        diff_hi[i] = scale * (out[i] - out[i - 1]); + +    /* Stability insurance */ +    for (i = LP_ORDER; i<  LP_ORDER_16k - 1; i++) +        if (diff_hi[i] + diff_hi[i - 1]<  5.0) { +            if (diff_hi[i]>  diff_hi[i - 1]) { +                diff_hi[i - 1] = 5.0 - diff_hi[i]; +            } else +                diff_hi[i] = 5.0 - diff_hi[i - 1]; +        } + +    for (i = LP_ORDER - 1; i<  LP_ORDER_16k - 1; i++) +        out[i] = out[i - 1] + diff_hi[i] * (1.0f / (1<<  15));

Hmm, this hunk looks a lot like

ff_sort_nearly_sorted_floats(out, LP_ORDER_16k); ff_set_min_dist_lsf(out, 5.0f / (1<<  15)); for (i = LP_ORDER - 1; i<  LP_ORDER_16k - 1; i++)     out[i] = out[i - 1] + scale * (1.0f / (1<<  15)) * (out[i] - out[i-1]);

or something like that.

Yes, but the min_dist_lsf would have to be applied between out[i] and out[i-2] I guess. I can't think of how to simplify this hunk without changing the output.

...

+/** + * Apply to high-band samples a 15th order filter + * The filter characteristic depends on the given coefficients + * + * @param[out] out                 Buffer for filtered output + * @param[in] fir_coef             Filter coefficients + * @param[in,out] mem              State from last filtering (updated) + * @param[in] cp_gain              Compensation gain (usually the filter gain) + * @param[in] in                   Input speech data (high-band) + * + * @remark It is safe to pass the same array in in and out parameters + */ +static void hb_fir_filter(float *out, const float fir_coef[HB_FIR_SIZE + 1], +                          float mem[HB_FIR_SIZE], float cp_gain, const float *in) +{ +    int i, j; +    float data[AMRWB_SFR_SIZE_16k + HB_FIR_SIZE]; // past and current samples + +    memcpy(data, mem, HB_FIR_SIZE * sizeof(float)); + +    for (i = 0; i<  AMRWB_SFR_SIZE_16k; i++) +        data[i + HB_FIR_SIZE] = in[i] / cp_gain; + +    for (i = 0; i<  AMRWB_SFR_SIZE_16k; i++) { +        out[i] = 0.0; +        for (j = 0; j<= HB_FIR_SIZE; j++) +            out[i] += data[i + j] * fir_coef[j]; +    } + +    memcpy(mem, data + AMRWB_SFR_SIZE_16k, HB_FIR_SIZE * sizeof(float)); +}

This function does too much memcpy: 30+30+80 elements. I suggest that you use a buffer HB_FIR_SIZE elements larger where is is called, so that the memcpy(data, mem, etc) is not needed.

The problem is that this function input is the output from synthesis which is preceded by its memory. I cannot avoid the memcpy for both functions. Although I can make the memory buffer 80 elements larger to avoid the memcpy of 30 elements. This would also solve the problem with the memory update that I mention later. It would use more memory at the AMRWBContext though.

Another thing is that the scaling by cp_gain is not needed. You can replace it by multiplying the bpf_6_7_coef table by 4.

Don't you mean dividing by 4? Fixed

Finally, I would prefer if the function contain just the inner loop (leaving the memory updating for the caller). This way it is simpler to reuse it in other codecs in the future. Since it is also a very good target for optimization (AMRWB spend>  30% of the decoding time on it), it is nice to have a simple interface the day someone decides to optimize it.

Hmm, I guess I can't do it with the function as of now because the inputs I need for the memory update may have been overwritten after the function returns (if input == output).

...

+static int amrwb_decode_frame(AVCodecContext *avctx, void *data, int *data_size, +                              AVPacket *avpkt) +{ +    AMRWBContext *ctx  = avctx->priv_data; +    AMRWBFrame   *cf   =&ctx->frame; +    const uint8_t *buf = avpkt->data; +    int buf_size       = avpkt->size; +    float *buf_out = data; +    float spare_vector[AMRWB_SFR_SIZE];      // extra stack space to hold result from anti-sparseness processing +    float fixed_gain_factor;                 // fixed gain correction factor (gamma) +    float *synth_fixed_vector;               // pointer to the fixed vector that synthesis should use +    float synth_fixed_gain;                  // the fixed gain that synthesis should use +    float voice_fac, stab_fac;               // parameters used for gain smoothing +    float synth_exc[AMRWB_SFR_SIZE];         // post-processed excitation for synthesis +    float hb_exc[AMRWB_SFR_SIZE_16k];        // excitation for the high frequency band +    float hb_samples[AMRWB_SFR_SIZE_16k];    // filtered high-band samples from synthesis +    float hb_gain; +    int sub, i; + +    ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size);

Missing a call like

    if (buf_size<  expected_input_size) {         av_log(avctx, AV_LOG_ERROR,                "Frame too small (%d bytes). Truncated file?\n", buf_size);         *data_size = 0;         return buf_size;     }

To avoid segfaults for truncated files.

Fixed

...

+        /* Add the low and high frequency bands */ +        for (i = 0; i<  AMRWB_SFR_SIZE_16k; i++) { +            // XXX: the low-band should really be upscaled by 2.0? This +            // way the output volume level seems doubled +            sub_buf[i] = (sub_buf[i] + hb_samples[i]) / 32768.0;

* (1.0f / (1<<  15));

Fixed -- Marcelo

On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote:

On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote:

...

Ok, fortunately I've found the bug!

[...]

...

So, here is how the decoder is now (I'm sending my commits to github also).

...

+/** Get x bits in the index interval [lsb,lsb+len-1] inclusive */ +#define BIT_STR(x,lsb,len) (((x)>> (lsb))& ((1<< (len)) - 1)) + +/** Get the bit at specified position */ +#define BIT_POS(x, p) (((x)>> (p))& 1)

If you move the #defines closer to where it is used, you improve the readability of your code.

Fixed

...

...

+ +/** + * Decodes quantized ISF vectors using 36-bit indices (6K60 mode only) + * + * @param[in] ind Array of 5 indices + * @param[out] isf_q Buffer for isf_q[LP_ORDER] + * + */ +static void decode_isf_indices_36b(uint16_t *ind, float *isf_q) { + int i; + + for (i = 0; i< 9; i++) { + isf_q[i] = dico1_isf[ind[0]][i] * (1.0f / (1<< 15)); + } + for (i = 0; i< 7; i++) { + isf_q[i + 9] = dico2_isf[ind[1]][i] * (1.0f / (1<< 15)); + } + for (i = 0; i< 5; i++) { + isf_q[i] += dico21_isf_36b[ind[2]][i] * (1.0f / (1<< 15)); + } + for (i = 0; i< 4; i++) { + isf_q[i + 5] += dico22_isf_36b[ind[3]][i] * (1.0f / (1<< 15)); + } + for (i = 0; i< 7; i++) { + isf_q[i + 9] += dico23_isf_36b[ind[4]][i] * (1.0f / (1<< 15)); + } +}

I think this would be more readable if the (1.0f / (1<< 15)) rescaling would be done in isf_add_mean_and_past().

Hmm, it improves readability at this part but I think that way the output for decode_isf_indices_36b() and the input for isf_add_mean_and_past() would have a strange meaning, if you know what I mean.

ok, makes sense...

...

...

+ +/** + * Find the pitch vector by interpolating the past excitation at the + * pitch delay, which is obtained in this function + * + * @param[in,out] ctx The context + * @param[in] amr_subframe Current subframe data + * @param[in] subframe Current subframe index (0 to 3) + */ +static void decode_pitch_vector(AMRWBContext *ctx, + const AMRWBSubFrame *amr_subframe, + const int subframe) +{ + int pitch_lag_int, pitch_lag_frac; + int i; + float *exc = ctx->excitation; + enum Mode mode = ctx->fr_cur_mode; + + if (mode<= MODE_8k85) { + decode_pitch_lag_low(&pitch_lag_int,&pitch_lag_frac, amr_subframe->adap, +&ctx->base_pitch_lag, subframe, mode); + } else + decode_pitch_lag_high(&pitch_lag_int,&pitch_lag_frac, amr_subframe->adap, +&ctx->base_pitch_lag, subframe); + + ctx->pitch_lag_int = pitch_lag_int;

...

+ pitch_lag_int += (pitch_lag_frac< 0 ? -1 : 0) + (pitch_lag_frac ? 1 : 0);

pitch_lag_int += pitch_lag_frac ? FFSIGN(pitch_lag_frac) : 0;

I don't see if I understand this correctly, see my "truth table" below: pitch_lag_frac | mine_RHS | your_RHS <0 | 0 | -1 =0 | 0 | 0

...

0 | 1 | 1

So, the simplified form would be this?

pitch_lag_int += pitch_lag_frac> 0 ? 1 : 0;

or, as Michael suggested pitch_lag_int += pitch_lag_frac > 0;

...

...

+/** + * Reduce fixed vector sparseness by smoothing with one of three IR filters + * Also known as "adaptive phase dispersion" + * + * @param[in] ctx The context + * @param[in,out] fixed_vector Unfiltered fixed vector + * @param[out] buf Space for modified vector if necessary + * + * @return The potentially overwritten filtered fixed vector address + */ +static float *anti_sparseness(AMRWBContext *ctx, + float *fixed_vector, float *buf) +{ + int ir_filter_nr; + + if (ctx->fr_cur_mode> MODE_8k85) // no filtering in higher modes + return fixed_vector; + + if (ctx->pitch_gain[0]< 0.6) { + ir_filter_nr = 0; // strong filtering + } else if (ctx->pitch_gain[0]< 0.9) { + ir_filter_nr = 1; // medium filtering + } else + ir_filter_nr = 2; // no filtering + + /* detect 'onset' */ + if (ctx->fixed_gain[0]> 3.0 * ctx->fixed_gain[1]) { + if (ir_filter_nr< 2) + ir_filter_nr++; + } else { + int i, count = 0; + + for (i = 0; i< 6; i++) + if (ctx->pitch_gain[i]< 0.6) + count++; + + if (count> 2) + ir_filter_nr = 0; + + if (ir_filter_nr> ctx->prev_ir_filter_nr + 1) + ir_filter_nr--; + } + + /* update ir filter strength history */ + ctx->prev_ir_filter_nr = ir_filter_nr; + + ir_filter_nr += (ctx->fr_cur_mode == MODE_8k85 ? 1 : 0); + + if (ir_filter_nr< 2) { + int i, j; + const float *coef = ir_filters_lookup[ir_filter_nr]; + + /* Circular convolution code in the reference + * decoder was modified to avoid using one + * extra array. The filtered vector is given by: + * + * c2(n) = sum(i,0,len-1){ c(i) * coef( (n - i + len) % len ) } + */ + + memset(buf, 0, sizeof(float) * AMRWB_SFR_SIZE); + for (i = 0; i< AMRWB_SFR_SIZE; i++)

...

+ if (fixed_vector[i]) { + int li = AMRWB_SFR_SIZE - i; + + for (j = 0; j< li; j++) + buf[i + j] += fixed_vector[i] * coef[j]; + + for (j = 0; j< i; j++) + buf[j] += fixed_vector[i] * coef[j + li]; + }

Can celp_filters.c:ff_celp_circ_addf() simplify this?

I already gave some thought to that but I couldn't figure out how to use ff_celp_circ_addf() there. I wrote the algorithm the simplest way I could think of.

Try for (i = 0; i < AMRWB_SFR_SIZE; i++) if (fixed_vector[i]) ff_celp_circ_addf(buf, buf, coef, i, fixed_vector[i], AMRWB_SFR_SIZE);

...

...

+/** + * Extrapolate a ISF vector to the 16kHz range (20th order LP) + * used at mode 6k60 LP filter for the high frequency band + * + * @param[out] out Buffer for extrapolated isf + * @param[in] isf Input isf vector + */ +static void extrapolate_isf(float out[LP_ORDER_16k], float isf[LP_ORDER]) +{ + float diff_isf[LP_ORDER - 2], diff_mean; + float *diff_hi = diff_isf - LP_ORDER + 1; // diff array for extrapolated indices + float corr_lag[3]; + float est, scale; + int i, i_max_corr; + + memcpy(out, isf, (LP_ORDER - 1) * sizeof(float)); + out[LP_ORDER_16k - 1] = isf[LP_ORDER - 1]; + + /* Calculate the difference vector */ + for (i = 0; i< LP_ORDER - 2; i++) + diff_isf[i] = isf[i + 1] - isf[i]; + + diff_mean = 0.0; + for (i = 2; i< LP_ORDER - 2; i++) + diff_mean += diff_isf[i] / (LP_ORDER - 4);

diff_mean += diff_isf[i] * (1.0f / (LP_ORDER - 4));

Fixed

...

...

+ for (i = LP_ORDER - 1; i< LP_ORDER_16k - 1; i++) + diff_hi[i] = scale * (out[i] - out[i - 1]); + + /* Stability insurance */ + for (i = LP_ORDER; i< LP_ORDER_16k - 1; i++) + if (diff_hi[i] + diff_hi[i - 1]< 5.0) { + if (diff_hi[i]> diff_hi[i - 1]) { + diff_hi[i - 1] = 5.0 - diff_hi[i]; + } else + diff_hi[i] = 5.0 - diff_hi[i - 1]; + } + + for (i = LP_ORDER - 1; i< LP_ORDER_16k - 1; i++) + out[i] = out[i - 1] + diff_hi[i] * (1.0f / (1<< 15));

Hmm, this hunk looks a lot like

ff_sort_nearly_sorted_floats(out, LP_ORDER_16k); ff_set_min_dist_lsf(out, 5.0f / (1<< 15)); for (i = LP_ORDER - 1; i< LP_ORDER_16k - 1; i++) out[i] = out[i - 1] + scale * (1.0f / (1<< 15)) * (out[i] - out[i-1]);

or something like that.

Yes, but the min_dist_lsf would have to be applied between out[i] and out[i-2] I guess. I can't think of how to simplify this hunk without changing the output.

...

...

+/** + * Apply to high-band samples a 15th order filter + * The filter characteristic depends on the given coefficients + * + * @param[out] out Buffer for filtered output + * @param[in] fir_coef Filter coefficients + * @param[in,out] mem State from last filtering (updated) + * @param[in] cp_gain Compensation gain (usually the filter gain) + * @param[in] in Input speech data (high-band) + * + * @remark It is safe to pass the same array in in and out parameters + */ +static void hb_fir_filter(float *out, const float fir_coef[HB_FIR_SIZE + 1], + float mem[HB_FIR_SIZE], float cp_gain, const float *in) +{ + int i, j; + float data[AMRWB_SFR_SIZE_16k + HB_FIR_SIZE]; // past and current samples + + memcpy(data, mem, HB_FIR_SIZE * sizeof(float)); + + for (i = 0; i< AMRWB_SFR_SIZE_16k; i++) + data[i + HB_FIR_SIZE] = in[i] / cp_gain; + + for (i = 0; i< AMRWB_SFR_SIZE_16k; i++) { + out[i] = 0.0; + for (j = 0; j<= HB_FIR_SIZE; j++) + out[i] += data[i + j] * fir_coef[j]; + } + + memcpy(mem, data + AMRWB_SFR_SIZE_16k, HB_FIR_SIZE * sizeof(float)); +}

This function does too much memcpy: 30+30+80 elements. I suggest that you use a buffer HB_FIR_SIZE elements larger where is is called, so that the memcpy(data, mem, etc) is not needed.

The problem is that this function input is the output from synthesis which is preceded by its memory. I cannot avoid the memcpy for both functions. Although I can make the memory buffer 80 elements larger to avoid the memcpy of 30 elements. This would also solve the problem with the memory update that I mention later. It would use more memory at the AMRWBContext though.

No, its ok as is then.

...

Another thing is that the scaling by cp_gain is not needed. You can replace it by multiplying the bpf_6_7_coef table by 4.

Don't you mean dividing by 4? Fixed

Sure. -Vitor

On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote:

On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote:

...

On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> wrote:

...

On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote:

...

[...]

...

...

...

Can celp_filters.c:ff_celp_circ_addf() simplify this?

I already gave some thought to that but I couldn't figure out how to use ff_celp_circ_addf() there. I wrote the algorithm the simplest way I could think of.

Try

for (i = 0; i< AMRWB_SFR_SIZE; i++) if (fixed_vector[i]) ff_celp_circ_addf(buf, buf, coef, i, fixed_vector[i], AMRWB_SFR_SIZE);

That's right indeed, thanks!

/* * AMR wideband decoder * Copyright (c) 2010 Marcelo Galvao Povoa * * This file is part of FFmpeg. * * FFmpeg is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License as published by the Free Software Foundation; either * version 2.1 of the License, or (at your option) any later version. * * FFmpeg is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A particular PURPOSE. See the GNU * Lesser General Public License for more details. * * You should have received a copy of the GNU Lesser General Public * License along with FFmpeg; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */

#include "libavutil/lfg.h"

#include "avcodec.h" #include "get_bits.h" #include "lsp.h" #include "celp_math.h" #include "celp_filters.h" #include "acelp_filters.h" #include "acelp_vectors.h" #include "acelp_pitch_delay.h"

#include "amrwbdata.h"

No need to include amr.h?

typedef struct { AMRWBFrame frame; ///< AMRWB parameters decoded from bitstream enum Mode fr_cur_mode; ///< mode index of current frame uint8_t fr_quality; ///< frame quality index (FQI) float isf_cur[LP_ORDER]; ///< working ISF vector from current frame float isf_q_past[LP_ORDER]; ///< quantized ISF vector of the previous frame float isf_past_final[LP_ORDER]; ///< final processed ISF vector of the previous frame double isp[4][LP_ORDER]; ///< ISP vectors from current frame double isp_sub4_past[LP_ORDER]; ///< ISP vector for the 4th subframe of the previous frame

float lp_coef[4][LP_ORDER]; ///< Linear Prediction Coefficients from ISP vector

uint8_t base_pitch_lag; ///< integer part of pitch lag for the next relative subframe uint8_t pitch_lag_int; ///< integer part of pitch lag of the previous subframe

float excitation_buf[AMRWB_P_DELAY_MAX + LP_ORDER + 2 + AMRWB_SFR_SIZE]; ///< current excitation and all necessary excitation history float *excitation; ///< points to current excitation in excitation_buf[]

float pitch_vector[AMRWB_SFR_SIZE]; ///< adaptive codebook (pitch) vector for current subframe float fixed_vector[AMRWB_SFR_SIZE]; ///< algebraic codebook (fixed) vector for current subframe

float prediction_error[4]; ///< quantified prediction errors {20log10(^gamma_gc)} for previous four subframes float pitch_gain[6]; ///< quantified pitch gains for the current and previous five subframes float fixed_gain[2]; ///< quantified fixed gains for the current and previous subframes

float tilt_coef; ///< {beta_1} related to the voicing of the previous subframe

float prev_sparse_fixed_gain; ///< previous fixed gain; used by anti-sparseness to determine "onset" uint8_t prev_ir_filter_nr; ///< previous impulse response filter "impNr": 0 - strong, 1 - medium, 2 - none float prev_tr_gain; ///< previous initial gain used by noise enhancer for threshold

float samples_az[LP_ORDER + AMRWB_SFR_SIZE]; ///< low-band samples and memory from synthesis at 12.8kHz float samples_up[UPS_MEM_SIZE + AMRWB_SFR_SIZE]; ///< low-band samples and memory processed for upsampling float samples_hb[LP_ORDER_16k + AMRWB_SFR_SIZE_16k]; ///< high-band samples and memory from synthesis at 16kHz

float hpf_31_mem[4], hpf_400_mem[4]; ///< previous values in the high pass filters

I think now just two elements for the filter memory should be enough.

diff --git a/libavcodec/amrwbdata.h b/libavcodec/amrwbdata.h new file mode 100644 index 0000000..6344763 --- /dev/null +++ b/libavcodec/amrwbdata.h

/** [i][j] is the number of pulses present in track j at mode i */ static const int pulses_nb_per_mode_tr[][4] = { {1, 1, 0, 0}, {1, 1, 1, 1}, {2, 2, 2, 2}, {3, 3, 2, 2}, {3, 3, 3, 3}, {4, 4, 4, 4}, {5, 5, 4, 4}, {6, 6, 6, 6}, {6, 6, 6, 6} };

uint8_t? And I have no further comments :D -Vitor

2010/9/11 Marcelo Galvão Póvoa <marspeoplester@gmail.com>:

On 11 September 2010 12:37, Vitor Sessak <vitor1001@gmail.com> wrote:

...

On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote:

...

On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote:

...

On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote: >

[...]

...

...

...

...

Can celp_filters.c:ff_celp_circ_addf() simplify this?

I already gave some thought to that but I couldn't figure out how to use ff_celp_circ_addf() there. I wrote the algorithm the simplest way I could think of.

Try

       for (i = 0; i<  AMRWB_SFR_SIZE; i++)            if (fixed_vector[i])                ff_celp_circ_addf(buf, buf, coef, i, fixed_vector[i],                                  AMRWB_SFR_SIZE);

That's right indeed, thanks!

...

/*  * AMR wideband decoder  * Copyright (c) 2010 Marcelo Galvao Povoa  *  * This file is part of FFmpeg.  *  * FFmpeg is free software; you can redistribute it and/or  * modify it under the terms of the GNU Lesser General Public  * License as published by the Free Software Foundation; either  * version 2.1 of the License, or (at your option) any later version.  *  * FFmpeg is distributed in the hope that it will be useful,  * but WITHOUT ANY WARRANTY; without even the implied warranty of  * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See the GNU  * Lesser General Public License for more details.  *  * You should have received a copy of the GNU Lesser General Public  * License along with FFmpeg; if not, write to the Free Software  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA  */

#include "libavutil/lfg.h"

#include "avcodec.h" #include "get_bits.h" #include "lsp.h" #include "celp_math.h" #include "celp_filters.h" #include "acelp_filters.h" #include "acelp_vectors.h" #include "acelp_pitch_delay.h"

#include "amrwbdata.h"

No need to include amr.h?

Oops, fixed

Sorry, I forgot to define the table type before the include. Fixed -- Marcelo

On 11 September 2010 18:54, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/11/2010 06:08 PM, Marcelo Galvão Póvoa wrote:

...

2010/9/11 Marcelo Galvão Póvoa<marspeoplester@gmail.com>:

...

On 11 September 2010 12:37, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote:

...

On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote: > > On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> >  wrote: >> >> On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote: >>>

[...]

...

...

>> Can celp_filters.c:ff_celp_circ_addf() simplify this? >> > > I already gave some thought to that but I couldn't figure out how to > use ff_celp_circ_addf() there. I wrote the algorithm the simplest way > I could think of.

Try

       for (i = 0; i<    AMRWB_SFR_SIZE; i++)            if (fixed_vector[i])                ff_celp_circ_addf(buf, buf, coef, i, fixed_vector[i],                                  AMRWB_SFR_SIZE);

That's right indeed, thanks!

...

/*  * AMR wideband decoder  * Copyright (c) 2010 Marcelo Galvao Povoa  *  * This file is part of FFmpeg.  *  * FFmpeg is free software; you can redistribute it and/or  * modify it under the terms of the GNU Lesser General Public  * License as published by the Free Software Foundation; either  * version 2.1 of the License, or (at your option) any later version.  *  * FFmpeg is distributed in the hope that it will be useful,  * but WITHOUT ANY WARRANTY; without even the implied warranty of  * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See the GNU  * Lesser General Public License for more details.  *  * You should have received a copy of the GNU Lesser General Public  * License along with FFmpeg; if not, write to the Free Software  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA  */

#include "libavutil/lfg.h"

#include "avcodec.h" #include "get_bits.h" #include "lsp.h" #include "celp_math.h" #include "celp_filters.h" #include "acelp_filters.h" #include "acelp_vectors.h" #include "acelp_pitch_delay.h"

#include "amrwbdata.h"

No need to include amr.h?

Oops, fixed

Sorry, I forgot to define the table type before the include. Fixed

Just one last think I just saw:

...

   ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size);    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7) >> 3) + 1;

   if (buf_size < expected_fr_size) {        av_log(avctx, AV_LOG_ERROR,            "Frame too small (%d bytes). Truncated file?\n", buf_size);        *data_size = 0;        return buf_size;    }

This check is not useful like that. Imagine that buf_size == 1. Then, unpack_bitstream will try to read buf[2] and will segfault. This check should be done before the call of ff_amr_bit_reorder().

I thought about that, but how I would calculate expected_fr_size if I don't know of which mode the frame is? One solution would be doing this check inside ff_amr_bit_reorder() just after decoding the header. -- Marcelo

On 12 September 2010 06:28, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/12/2010 12:25 AM, Marcelo Galvão Póvoa wrote:

...

On 11 September 2010 18:54, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/11/2010 06:08 PM, Marcelo Galvão Póvoa wrote:

...

2010/9/11 Marcelo Galvão Póvoa<marspeoplester@gmail.com>:

...

On 11 September 2010 12:37, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote: > > On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com> >  wrote: >> >> On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote: >>> >>> On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> >>>  wrote: >>>> >>>> On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote: >>>>>

[...]

>>>> Can celp_filters.c:ff_celp_circ_addf() simplify this? >>>> >>> >>> I already gave some thought to that but I couldn't figure out how >>> to >>> use ff_celp_circ_addf() there. I wrote the algorithm the simplest >>> way >>> I could think of. >> >> Try >> >>        for (i = 0; i<      AMRWB_SFR_SIZE; i++) >>            if (fixed_vector[i]) >>                ff_celp_circ_addf(buf, buf, coef, i, fixed_vector[i], >>                                  AMRWB_SFR_SIZE); >> >> > > That's right indeed, thanks!

> /* >  * AMR wideband decoder >  * Copyright (c) 2010 Marcelo Galvao Povoa >  * >  * This file is part of FFmpeg. >  * >  * FFmpeg is free software; you can redistribute it and/or >  * modify it under the terms of the GNU Lesser General Public >  * License as published by the Free Software Foundation; either >  * version 2.1 of the License, or (at your option) any later version. >  * >  * FFmpeg is distributed in the hope that it will be useful, >  * but WITHOUT ANY WARRANTY; without even the implied warranty of >  * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See the GNU >  * Lesser General Public License for more details. >  * >  * You should have received a copy of the GNU Lesser General Public >  * License along with FFmpeg; if not, write to the Free Software >  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA >  */ > > #include "libavutil/lfg.h" > > #include "avcodec.h" > #include "get_bits.h" > #include "lsp.h" > #include "celp_math.h" > #include "celp_filters.h" > #include "acelp_filters.h" > #include "acelp_vectors.h" > #include "acelp_pitch_delay.h" > > #include "amrwbdata.h"

No need to include amr.h?

Oops, fixed

Sorry, I forgot to define the table type before the include. Fixed

Just one last think I just saw:

...

   ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size);    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>  3) + 1;

   if (buf_size<  expected_fr_size) {        av_log(avctx, AV_LOG_ERROR,            "Frame too small (%d bytes). Truncated file?\n", buf_size);        *data_size = 0;        return buf_size;    }

This check is not useful like that. Imagine that buf_size == 1. Then, unpack_bitstream will try to read buf[2] and will segfault. This check should be done before the call of ff_amr_bit_reorder().

I thought about that, but how I would calculate expected_fr_size if I don't know of which mode the frame is? One solution would be doing this check inside ff_amr_bit_reorder() just after decoding the header.

Another solution would be to get rid of the unpack_bitstrem() function and move its code to amrwb_decode_frame(). It is already almost a wrapper around ff_amr_bit_reorder(). It would also allows to avoid checking (ctx->fr_cur_mode < MODE_SID) twice...

Interesting, but how about instead of moving all the code to amrwb_decode_frame(), we keep a function to decode the header since there are more than one type of it. I wrote the simpler (and most common I guess) "MIME/storage" header format but in case someone wants to implement the others this way would be easier. See the patch I attached. -- Marcelo

On 12 September 2010 14:39, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/12/2010 04:34 PM, Marcelo Galvão Póvoa wrote:

...

On 12 September 2010 06:28, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/12/2010 12:25 AM, Marcelo Galvão Póvoa wrote:

...

On 11 September 2010 18:54, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

On 09/11/2010 06:08 PM, Marcelo Galvão Póvoa wrote:

...

2010/9/11 Marcelo Galvão Póvoa<marspeoplester@gmail.com>: > > On 11 September 2010 12:37, Vitor Sessak<vitor1001@gmail.com> >  wrote: >> >> On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote: >>> >>> On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com> >>>  wrote: >>>> >>>> On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote: >>>>> >>>>> On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> >>>>>  wrote: >>>>>> >>>>>> On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote: >>>>>>> >> >> [...] >> >>>>>> Can celp_filters.c:ff_celp_circ_addf() simplify this? >>>>>> >>>>> >>>>> I already gave some thought to that but I couldn't figure out how >>>>> to >>>>> use ff_celp_circ_addf() there. I wrote the algorithm the simplest >>>>> way >>>>> I could think of. >>>> >>>> Try >>>> >>>>        for (i = 0; i<        AMRWB_SFR_SIZE; i++) >>>>            if (fixed_vector[i]) >>>>                ff_celp_circ_addf(buf, buf, coef, i, >>>> fixed_vector[i], >>>>                                  AMRWB_SFR_SIZE); >>>> >>>> >>> >>> That's right indeed, thanks! >> >>> /* >>>  * AMR wideband decoder >>>  * Copyright (c) 2010 Marcelo Galvao Povoa >>>  * >>>  * This file is part of FFmpeg. >>>  * >>>  * FFmpeg is free software; you can redistribute it and/or >>>  * modify it under the terms of the GNU Lesser General Public >>>  * License as published by the Free Software Foundation; either >>>  * version 2.1 of the License, or (at your option) any later >>> version. >>>  * >>>  * FFmpeg is distributed in the hope that it will be useful, >>>  * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>  * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See the >>> GNU >>>  * Lesser General Public License for more details. >>>  * >>>  * You should have received a copy of the GNU Lesser General Public >>>  * License along with FFmpeg; if not, write to the Free Software >>>  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>> 02110-1301 USA >>>  */ >>> >>> #include "libavutil/lfg.h" >>> >>> #include "avcodec.h" >>> #include "get_bits.h" >>> #include "lsp.h" >>> #include "celp_math.h" >>> #include "celp_filters.h" >>> #include "acelp_filters.h" >>> #include "acelp_vectors.h" >>> #include "acelp_pitch_delay.h" >>> >>> #include "amrwbdata.h" >> >> No need to include amr.h? >> > > Oops, fixed >

Sorry, I forgot to define the table type before the include. Fixed

Just one last think I just saw:

...

   ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size);    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>    3) + 1;

   if (buf_size<    expected_fr_size) {        av_log(avctx, AV_LOG_ERROR,            "Frame too small (%d bytes). Truncated file?\n", buf_size);        *data_size = 0;        return buf_size;    }

This check is not useful like that. Imagine that buf_size == 1. Then, unpack_bitstream will try to read buf[2] and will segfault. This check should be done before the call of ff_amr_bit_reorder().

I thought about that, but how I would calculate expected_fr_size if I don't know of which mode the frame is? One solution would be doing this check inside ff_amr_bit_reorder() just after decoding the header.

Another solution would be to get rid of the unpack_bitstrem() function and move its code to amrwb_decode_frame(). It is already almost a wrapper around ff_amr_bit_reorder(). It would also allows to avoid checking (ctx->fr_cur_mode<  MODE_SID) twice...

Interesting, but how about instead of moving all the code to amrwb_decode_frame(), we keep a function to decode the header since there are more than one type of it. I wrote the simpler (and most common I guess) "MIME/storage" header format but in case someone wants to implement the others this way would be easier. See the patch I attached.

I like it. Just a last nitpick:

...

+    header_size      = decode_mime_header(ctx, buf); +    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7) >> 3) + 1; + +    if (buf_size < expected_fr_size) { +        av_log(avctx, AV_LOG_ERROR, +            "Frame too small (%d bytes). Truncated file?\n", buf_size); +        *data_size = 0; +        return buf_size; +    } +

...

+    if (!ctx->fr_quality || ctx->fr_cur_mode > MODE_SID) { +        av_log(avctx, AV_LOG_ERROR, "Encountered a bad or corrupted frame\n"); +    }

Is there any chance of decoding that frame if ctx->fr_cur_mode > MODE_SID? Better to bail out with an error in this case.

...

+    if (ctx->fr_cur_mode < MODE_SID) { /* Normal speech frame */ +        ff_amr_bit_reorder((uint16_t *) &ctx->frame, sizeof(AMRWBFrame), +            buf + header_size, amr_bit_orderings_by_mode[ctx->fr_cur_mode]); +    } +    else if (ctx->fr_cur_mode == MODE_SID) { /* Comfort noise frame */ +        av_log_missing_feature(avctx, "SID mode", 1); +        return -1; +    }

I think it all be clearer in the following way:

   header_size      = decode_mime_header(ctx, buf);    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7) >> 3) + 1;

   if (buf_size < expected_fr_size) {        av_log(avctx, AV_LOG_ERROR,            "Frame too small (%d bytes). Truncated file?\n", buf_size);        *data_size = 0;        return buf_size;    }

   if (!ctx->fr_quality || ctx->fr_cur_mode > MODE_SID)        av_log(avctx, AV_LOG_ERROR, "Encountered a bad or corrupted frame\n");

   if (ctx->fr_cur_mode == MODE_SID) /* Comfort noise frame */        av_log_missing_feature(avctx, "SID mode", 1);

   if (ctx->fr_cur_mode >= MODE_SID)        return -1;

  ff_amr_bit_reorder((uint16_t *) &ctx->frame, sizeof(AMRWBFrame),        buf + header_size, amr_bit_orderings_by_mode[ctx->fr_cur_mode]);

In that way, the ff_amr_bit_reorder() call is not in the middle of the error handling. Finally, after fixing this, if Rob agree, I think you can move this discussion to ffmpeg-devel.

Fixed. Ok, there are still some of those "XXX" notes however, did you check them? -- Marcelo

On 12 September 2010 21:00, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/13/2010 12:21 AM, Marcelo Galvão Póvoa wrote:

...

On 12 September 2010 14:39, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/12/2010 04:34 PM, Marcelo Galvão Póvoa wrote:

...

On 12 September 2010 06:28, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

On 09/12/2010 12:25 AM, Marcelo Galvão Póvoa wrote:

...

On 11 September 2010 18:54, Vitor Sessak<vitor1001@gmail.com>  wrote: > > On 09/11/2010 06:08 PM, Marcelo Galvão Póvoa wrote: >> >> 2010/9/11 Marcelo Galvão Póvoa<marspeoplester@gmail.com>: >>> >>> On 11 September 2010 12:37, Vitor Sessak<vitor1001@gmail.com> >>>  wrote: >>>> >>>> On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote: >>>>> >>>>> On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com> >>>>>  wrote: >>>>>> >>>>>> On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote: >>>>>>> >>>>>>> On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> >>>>>>>  wrote: >>>>>>>> >>>>>>>> On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote: >>>>>>>>> >>>> >>>> [...] >>>> >>>>>>>> Can celp_filters.c:ff_celp_circ_addf() simplify this? >>>>>>>> >>>>>>> >>>>>>> I already gave some thought to that but I couldn't figure out >>>>>>> how >>>>>>> to >>>>>>> use ff_celp_circ_addf() there. I wrote the algorithm the >>>>>>> simplest >>>>>>> way >>>>>>> I could think of. >>>>>> >>>>>> Try >>>>>> >>>>>>        for (i = 0; i<          AMRWB_SFR_SIZE; i++) >>>>>>            if (fixed_vector[i]) >>>>>>                ff_celp_circ_addf(buf, buf, coef, i, >>>>>> fixed_vector[i], >>>>>>                                  AMRWB_SFR_SIZE); >>>>>> >>>>>> >>>>> >>>>> That's right indeed, thanks! >>>> >>>>> /* >>>>>  * AMR wideband decoder >>>>>  * Copyright (c) 2010 Marcelo Galvao Povoa >>>>>  * >>>>>  * This file is part of FFmpeg. >>>>>  * >>>>>  * FFmpeg is free software; you can redistribute it and/or >>>>>  * modify it under the terms of the GNU Lesser General Public >>>>>  * License as published by the Free Software Foundation; either >>>>>  * version 2.1 of the License, or (at your option) any later >>>>> version. >>>>>  * >>>>>  * FFmpeg is distributed in the hope that it will be useful, >>>>>  * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>  * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See the >>>>> GNU >>>>>  * Lesser General Public License for more details. >>>>>  * >>>>>  * You should have received a copy of the GNU Lesser General >>>>> Public >>>>>  * License along with FFmpeg; if not, write to the Free Software >>>>>  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>>>> 02110-1301 USA >>>>>  */ >>>>> >>>>> #include "libavutil/lfg.h" >>>>> >>>>> #include "avcodec.h" >>>>> #include "get_bits.h" >>>>> #include "lsp.h" >>>>> #include "celp_math.h" >>>>> #include "celp_filters.h" >>>>> #include "acelp_filters.h" >>>>> #include "acelp_vectors.h" >>>>> #include "acelp_pitch_delay.h" >>>>> >>>>> #include "amrwbdata.h" >>>> >>>> No need to include amr.h? >>>> >>> >>> Oops, fixed >>> >> >> Sorry, I forgot to define the table type before the include. Fixed > > Just one last think I just saw: > > >>    ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size); >>    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>      3) >> + >> 1; >> >>    if (buf_size<      expected_fr_size) { >>        av_log(avctx, AV_LOG_ERROR, >>            "Frame too small (%d bytes). Truncated file?\n", >> buf_size); >>        *data_size = 0; >>        return buf_size; >>    } > > This check is not useful like that. Imagine that buf_size == 1. Then, > unpack_bitstream will try to read buf[2] and will segfault. This > check > should be done before the call of ff_amr_bit_reorder(). >

I thought about that, but how I would calculate expected_fr_size if I don't know of which mode the frame is? One solution would be doing this check inside ff_amr_bit_reorder() just after decoding the header.

Another solution would be to get rid of the unpack_bitstrem() function and move its code to amrwb_decode_frame(). It is already almost a wrapper around ff_amr_bit_reorder(). It would also allows to avoid checking (ctx->fr_cur_mode<    MODE_SID) twice...

Interesting, but how about instead of moving all the code to amrwb_decode_frame(), we keep a function to decode the header since there are more than one type of it. I wrote the simpler (and most common I guess) "MIME/storage" header format but in case someone wants to implement the others this way would be easier. See the patch I attached.

I like it. Just a last nitpick:

...

+    header_size      = decode_mime_header(ctx, buf); +    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>  3) + 1; + +    if (buf_size<  expected_fr_size) { +        av_log(avctx, AV_LOG_ERROR, +            "Frame too small (%d bytes). Truncated file?\n", buf_size); +        *data_size = 0; +        return buf_size; +    } +

...

+    if (!ctx->fr_quality || ctx->fr_cur_mode>  MODE_SID) { +        av_log(avctx, AV_LOG_ERROR, "Encountered a bad or corrupted frame\n"); +    }

Is there any chance of decoding that frame if ctx->fr_cur_mode>  MODE_SID? Better to bail out with an error in this case.

...

+    if (ctx->fr_cur_mode<  MODE_SID) { /* Normal speech frame */ +        ff_amr_bit_reorder((uint16_t *)&ctx->frame, sizeof(AMRWBFrame), +            buf + header_size, amr_bit_orderings_by_mode[ctx->fr_cur_mode]); +    } +    else if (ctx->fr_cur_mode == MODE_SID) { /* Comfort noise frame */ +        av_log_missing_feature(avctx, "SID mode", 1); +        return -1; +    }

I think it all be clearer in the following way:

   header_size      = decode_mime_header(ctx, buf);    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>  3) + 1;

   if (buf_size<  expected_fr_size) {        av_log(avctx, AV_LOG_ERROR,            "Frame too small (%d bytes). Truncated file?\n", buf_size);        *data_size = 0;        return buf_size;    }

   if (!ctx->fr_quality || ctx->fr_cur_mode>  MODE_SID)        av_log(avctx, AV_LOG_ERROR, "Encountered a bad or corrupted frame\n");

   if (ctx->fr_cur_mode == MODE_SID) /* Comfort noise frame */        av_log_missing_feature(avctx, "SID mode", 1);

   if (ctx->fr_cur_mode>= MODE_SID)        return -1;

  ff_amr_bit_reorder((uint16_t *)&ctx->frame, sizeof(AMRWBFrame),        buf + header_size, amr_bit_orderings_by_mode[ctx->fr_cur_mode]);

In that way, the ff_amr_bit_reorder() call is not in the middle of the error handling. Finally, after fixing this, if Rob agree, I think you can move this discussion to ffmpeg-devel.

Fixed.

Ok, there are still some of those "XXX" notes however, did you check them?

Most of them looks like differences between the spec and the ref decoder. Such things happens all the time, and one should follow the ref decoder in those cases. About voice_factor(), I understand that you tested this

Ok, already did that

function thoroughly when tracking that old quality-affecting bug and found no difference from the reference. Finally, about the truncation of the

About voice_factor(), the values it returns are not quite right, maybe it can be improved. But the stddev of the output between using my stddev and the one from reference is small: 0.38

excitation, this was done in AMRNB more to reduce the differences due to our implementation been floating-point based while the reference being fixed-point. But for AMRNB those differences were causing audible artifacts. If you want to be sure, you can check if the stddev with the pre-encoding input increases if you add such a truncation (this time using the right shift of -190)

Actually, the sample I've sent you (soc_pod.wav) was already using truncation. Testing with -190, this version is slightly better: 643.05 against 644.60 I think I'll keep it. -- Marcelo

On 17 September 2010 16:49, Vitor Sessak <vitor1001@gmail.com> wrote:

On 09/13/2010 01:37 PM, Marcelo Galvão Póvoa wrote:

...

On 12 September 2010 21:00, Vitor Sessak<vitor1001@gmail.com>  wrote:

...

On 09/13/2010 12:21 AM, Marcelo Galvão Póvoa wrote:

...

On 12 September 2010 14:39, Vitor Sessak<vitor1001@gmail.com>    wrote:

...

On 09/12/2010 04:34 PM, Marcelo Galvão Póvoa wrote:

...

On 12 September 2010 06:28, Vitor Sessak<vitor1001@gmail.com>  wrote: > > On 09/12/2010 12:25 AM, Marcelo Galvão Póvoa wrote: >> >> On 11 September 2010 18:54, Vitor Sessak<vitor1001@gmail.com> >>  wrote: >>> >>> On 09/11/2010 06:08 PM, Marcelo Galvão Póvoa wrote: >>>> >>>> 2010/9/11 Marcelo Galvão Póvoa<marspeoplester@gmail.com>: >>>>> >>>>> On 11 September 2010 12:37, Vitor Sessak<vitor1001@gmail.com> >>>>>  wrote: >>>>>> >>>>>> On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote: >>>>>>> >>>>>>> On 9 September 2010 16:11, Vitor Sessak<vitor1001@gmail.com> >>>>>>>  wrote: >>>>>>>> >>>>>>>> On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote: >>>>>>>>> >>>>>>>>> On 6 September 2010 11:31, Vitor Sessak<vitor1001@gmail.com> >>>>>>>>>  wrote: >>>>>>>>>> >>>>>>>>>> On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote: >>>>>>>>>>> >>>>>> >>>>>> [...] >>>>>> >>>>>>>>>> Can celp_filters.c:ff_celp_circ_addf() simplify this? >>>>>>>>>> >>>>>>>>> >>>>>>>>> I already gave some thought to that but I couldn't figure out >>>>>>>>> how >>>>>>>>> to >>>>>>>>> use ff_celp_circ_addf() there. I wrote the algorithm the >>>>>>>>> simplest >>>>>>>>> way >>>>>>>>> I could think of. >>>>>>>> >>>>>>>> Try >>>>>>>> >>>>>>>>        for (i = 0; i<            AMRWB_SFR_SIZE; i++) >>>>>>>>            if (fixed_vector[i]) >>>>>>>>                ff_celp_circ_addf(buf, buf, coef, i, >>>>>>>> fixed_vector[i], >>>>>>>>                                  AMRWB_SFR_SIZE); >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> That's right indeed, thanks! >>>>>> >>>>>>> /* >>>>>>>  * AMR wideband decoder >>>>>>>  * Copyright (c) 2010 Marcelo Galvao Povoa >>>>>>>  * >>>>>>>  * This file is part of FFmpeg. >>>>>>>  * >>>>>>>  * FFmpeg is free software; you can redistribute it and/or >>>>>>>  * modify it under the terms of the GNU Lesser General Public >>>>>>>  * License as published by the Free Software Foundation; either >>>>>>>  * version 2.1 of the License, or (at your option) any later >>>>>>> version. >>>>>>>  * >>>>>>>  * FFmpeg is distributed in the hope that it will be useful, >>>>>>>  * but WITHOUT ANY WARRANTY; without even the implied warranty >>>>>>> of >>>>>>>  * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See >>>>>>> the >>>>>>> GNU >>>>>>>  * Lesser General Public License for more details. >>>>>>>  * >>>>>>>  * You should have received a copy of the GNU Lesser General >>>>>>> Public >>>>>>>  * License along with FFmpeg; if not, write to the Free >>>>>>> Software >>>>>>>  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, >>>>>>> MA >>>>>>> 02110-1301 USA >>>>>>>  */ >>>>>>> >>>>>>> #include "libavutil/lfg.h" >>>>>>> >>>>>>> #include "avcodec.h" >>>>>>> #include "get_bits.h" >>>>>>> #include "lsp.h" >>>>>>> #include "celp_math.h" >>>>>>> #include "celp_filters.h" >>>>>>> #include "acelp_filters.h" >>>>>>> #include "acelp_vectors.h" >>>>>>> #include "acelp_pitch_delay.h" >>>>>>> >>>>>>> #include "amrwbdata.h" >>>>>> >>>>>> No need to include amr.h? >>>>>> >>>>> >>>>> Oops, fixed >>>>> >>>> >>>> Sorry, I forgot to define the table type before the include. Fixed >>> >>> Just one last think I just saw: >>> >>> >>>>    ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size); >>>>    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>> >>>>  3) >>>> + >>>> 1; >>>> >>>>    if (buf_size<        expected_fr_size) { >>>>        av_log(avctx, AV_LOG_ERROR, >>>>            "Frame too small (%d bytes). Truncated file?\n", >>>> buf_size); >>>>        *data_size = 0; >>>>        return buf_size; >>>>    } >>> >>> This check is not useful like that. Imagine that buf_size == 1. >>> Then, >>> unpack_bitstream will try to read buf[2] and will segfault. This >>> check >>> should be done before the call of ff_amr_bit_reorder(). >>> >> >> I thought about that, but how I would calculate expected_fr_size if >> I >> don't know of which mode the frame is? One solution would be doing >> this check inside ff_amr_bit_reorder() just after decoding the >> header. > > Another solution would be to get rid of the unpack_bitstrem() > function > and > move its code to amrwb_decode_frame(). It is already almost a wrapper > around > ff_amr_bit_reorder(). It would also allows to avoid checking > (ctx->fr_cur_mode<      MODE_SID) twice... >

Interesting, but how about instead of moving all the code to amrwb_decode_frame(), we keep a function to decode the header since there are more than one type of it. I wrote the simpler (and most common I guess) "MIME/storage" header format but in case someone wants to implement the others this way would be easier. See the patch I attached.

I like it. Just a last nitpick:

...

+    header_size      = decode_mime_header(ctx, buf); +    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>    3) + 1; + +    if (buf_size<    expected_fr_size) { +        av_log(avctx, AV_LOG_ERROR, +            "Frame too small (%d bytes). Truncated file?\n", buf_size); +        *data_size = 0; +        return buf_size; +    } +

...

+    if (!ctx->fr_quality || ctx->fr_cur_mode>    MODE_SID) { +        av_log(avctx, AV_LOG_ERROR, "Encountered a bad or corrupted frame\n"); +    }

Is there any chance of decoding that frame if ctx->fr_cur_mode>  MODE_SID? Better to bail out with an error in this case.

...

+    if (ctx->fr_cur_mode<    MODE_SID) { /* Normal speech frame */ +        ff_amr_bit_reorder((uint16_t *)&ctx->frame, sizeof(AMRWBFrame), +            buf + header_size, amr_bit_orderings_by_mode[ctx->fr_cur_mode]); +    } +    else if (ctx->fr_cur_mode == MODE_SID) { /* Comfort noise frame */ +        av_log_missing_feature(avctx, "SID mode", 1); +        return -1; +    }

I think it all be clearer in the following way:

   header_size      = decode_mime_header(ctx, buf);    expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7)>>    3) + 1;

   if (buf_size<    expected_fr_size) {        av_log(avctx, AV_LOG_ERROR,            "Frame too small (%d bytes). Truncated file?\n", buf_size);        *data_size = 0;        return buf_size;    }

   if (!ctx->fr_quality || ctx->fr_cur_mode>    MODE_SID)        av_log(avctx, AV_LOG_ERROR, "Encountered a bad or corrupted frame\n");

   if (ctx->fr_cur_mode == MODE_SID) /* Comfort noise frame */        av_log_missing_feature(avctx, "SID mode", 1);

   if (ctx->fr_cur_mode>= MODE_SID)        return -1;

  ff_amr_bit_reorder((uint16_t *)&ctx->frame, sizeof(AMRWBFrame),        buf + header_size, amr_bit_orderings_by_mode[ctx->fr_cur_mode]);

In that way, the ff_amr_bit_reorder() call is not in the middle of the error handling. Finally, after fixing this, if Rob agree, I think you can move this discussion to ffmpeg-devel.

Fixed.

Ok, there are still some of those "XXX" notes however, did you check them?

Most of them looks like differences between the spec and the ref decoder. Such things happens all the time, and one should follow the ref decoder in those cases. About voice_factor(), I understand that you tested this

Ok, already did that

...

function thoroughly when tracking that old quality-affecting bug and found no difference from the reference. Finally, about the truncation of the

About voice_factor(), the values it returns are not quite right, maybe it can be improved. But the stddev of the output between using my stddev and the one from reference is small: 0.38

We can not hope of anything much better than 0.38 when comparing fixed-point with floats...

...

...

excitation, this was done in AMRNB more to reduce the differences due to our implementation been floating-point based while the reference being fixed-point. But for AMRNB those differences were causing audible artifacts. If you want to be sure, you can check if the stddev with the pre-encoding input increases if you add such a truncation (this time using the right shift of -190)

Actually, the sample I've sent you (soc_pod.wav) was already using truncation. Testing with -190, this version is slightly better: 643.05 against 644.60 I think I'll keep it.

So I'm favorable to it.

Also, please post your newest patch to -devel, let's get this reviewed and committed :-)

Ok, I've sent it but I received an email saying that since the message is too large, the list moderator should approve it. I'm waiting for it... =) -- Marcelo