On Sun, Jul 15, 2012 at 06:35:34PM -0400, Justin Ruggles wrote:
> Hi,
>
> On 07/14/2012 09:41 AM, Kostya Shishkov wrote:
> > Decoder is _not_ bitexact but hopefully is good enough.
> > Sent gzipped since the patch is rather large.
[...]
> > + Rate cur_rate;
>
> I think keeping the enums as enums and not typedef-ing them would make
> it easier to distinguish them from the typedef-ed structs.
changed (and got rid of camel-casing)
> > + uint8_t lsp_index[LSP_BANDS];
> > + int pitch_lag[2];
> > + int erased_frames;
> > +
> > + int16_t prev_lsp[LPC_ORDER];
> > + int16_t prev_excitation[PITCH_MAX];
> > + int16_t excitation[PITCH_MAX + FRAME_LEN];
> > + int16_t synth_mem[LPC_ORDER];
> > + int16_t fir_mem[LPC_ORDER];
> > + int iir_mem[LPC_ORDER];
> > +
> > + int random_seed;
> > + int interp_index;
> > + int interp_gain;
> > + int sid_gain;
> > + int cur_gain;
> > + int reflection_coef;
> > + int pf_gain;
> > + int postfilter;
> > +
> > + int16_t audio[FRAME_LEN + LPC_ORDER];
> > +} G723_1_Context;
> > +
> > +static av_cold int g723_1_decode_init(AVCodecContext *avctx)
> > +{
> > + G723_1_Context *p = avctx->priv_data;
> > +
> > + avctx->channel_layout = AV_CH_LAYOUT_MONO;
> > + avctx->sample_fmt = AV_SAMPLE_FMT_S16;
> > + avctx->channels = 1;
> > + avctx->sample_rate = 8000;
> > + p->pf_gain = 1 << 12;
> > +
> > + avcodec_get_frame_defaults(&p->frame);
> > + avctx->coded_frame = &p->frame;
> > +
> > + memcpy(p->prev_lsp, dc_lsp, LPC_ORDER * sizeof(int16_t));
>
> As a general comment, most of the places in this decoder that do
> memcpy() and similar things are using sizeof(type) instead of
> sizeof(*variable). I think we need to be using the latter.
changed
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * Unpack the frame into parameters.
> > + *
> > + * @param p the context
> > + * @param buf pointer to the input buffer
> > + * @param buf_size size of the input buffer
> > + */
> > +static int unpack_bitstream(G723_1_Context *p, const uint8_t *buf,
> > + int buf_size)
> > +{
> > + GetBitContext gb;
> > + int ad_cb_len;
> > + int temp, info_bits, i;
> > +
> > + init_get_bits(&gb, buf, buf_size * 8);
> > +
> > + /* Extract frame type and rate info */
> > + info_bits = get_bits(&gb, 2);
> > +
> > + if (info_bits == 3) {
> > + p->cur_frame_type = UntransmittedFrame;
> > + return 0;
> > + }
> > +
> > + /* Extract 24 bit lsp indices, 8 bit for each band */
> > + p->lsp_index[2] = get_bits(&gb, 8);
> > + p->lsp_index[1] = get_bits(&gb, 8);
> > + p->lsp_index[0] = get_bits(&gb, 8);
> > +
> > + if (info_bits == 2) {
> > + p->cur_frame_type = SIDFrame;
> > + p->subframe[0].amp_index = get_bits(&gb, 6);
> > + return 0;
> > + }
> > +
> > + /* Extract the info common to both rates */
> > + p->cur_rate = info_bits ? Rate5k3 : Rate6k3;
> > + p->cur_frame_type = ActiveFrame;
> > +
> > + p->pitch_lag[0] = get_bits(&gb, 7);
> > + if (p->pitch_lag[0] > 123) /* test if forbidden code */
> > + return -1;
>
> #define LAG_MAX 123 ?
dunno if it's worth it
> > + p->pitch_lag[0] += PITCH_MIN;
> > + p->subframe[1].ad_cb_lag = get_bits(&gb, 2);
> > +
> > + p->pitch_lag[1] = get_bits(&gb, 7);
> > + if (p->pitch_lag[1] > 123)
> > + return -1;
> [...]
> > +/**
> > + * Calculate the number of left-shifts required for normalizing the input.
> > + *
> > + * @param num input number
> > + * @param width width of the input, 16 bits(0) / 32 bits(1)
> > + */
> > +static int normalize_bits(int num, int width)
> > +{
> > + int i = 0;
> > + int bits = (width) ? 31 : 15;
>
> why pass 0/1 instead of 31/15? the places that call this function are
> just using 0 and 1 directly anyway.
done
> > + int limit = 1 << (bits - 1);
> > +
> > + if (num) {
> > + if (num == -1)
> > + return bits;
> > + if (num < 0)
> > + num = ~num;
> > + for (i = 0; num < limit; i++)
> > + num <<= 1;
> > + }
> > + return i;
> > +}
>
> couldn't the loop be replaced with (bits - 1 - av_log2(num))?
no, but it could be (and is) replaced with bits - av_log2(num)
> > +
> > +/**
> > + * Scale vector contents based on the largest of their absolutes.
> > + */
> > +static int scale_vector(int16_t *vector, int length)
> > +{
> > + int bits, scale, max = 0;
> > + int i;
> > +
> > + const int16_t shift_table[16] = {
> > + 0x0001, 0x0002, 0x0004, 0x0008, 0x0010, 0x0020, 0x0040, 0x0080,
> > + 0x0100, 0x0200, 0x0400, 0x0800, 0x1000, 0x2000, 0x4000, 0x7fff
> > + };
>
> should that table be static?
it shouldn't be at all IMO, it's just (bits == 15) ? 0x7FFF : (1 << bits)
> > +
> > + for (i = 0; i < length; i++)
> > + max = FFMAX(max, FFABS(vector[i]));
> > +
> > + bits = normalize_bits(max, 0);
> > + scale = shift_table[bits];
> > +
> > + for (i = 0; i < length; i++)
> > + vector[i] = (int16_t)(av_clipl_int32(vector[i] * scale << 1) >> 4);
>
> this looks wrong for a couple reasons.
>
> 1. av_clipl_int32() is for clipping 64-bit values to 32-bit, which won't
> happen with the math there. (-32768 * 32767 << 1) is in int32_t range.
>
> 2. the cast to int16_t is implementation-defined when the result is
> outside of 16-bit range, which certainly looks possible.
Wrong in this case because the second argument is calculated to make values
fill 16-bit range but not more.
I've simplified it a bit though.
> [...]
> > +static int dot_product(const int16_t *a, const int16_t *b, int length,
> > + int shift)
> > +{
> > + int i, sum = 0;
> > +
> > + for (i = 0; i < length; i++) {
> > + int64_t prod = av_clipl_int32(MUL64(a[i], b[i]) << shift);
> > + sum = av_clipl_int32(sum + prod);
> > + }
> > + return sum;
> > +}
>
> Is this clipping of the sum in each iteration part of some bit-exact
> behavior? If not, it might be faster to make sum 64-bit and clip only at
> the end.
I don't know how it's here but at least in G.722.1 it was so.
Thus left as is.
> > +
> > +/**
> > + * Generate adaptive codebook excitation.
> > + */
> > +static void gen_acb_excitation(int16_t *vector, int16_t *prev_excitation,
> > + int pitch_lag, G723_1_Subframe subfrm,
> > + Rate cur_rate)
> > +{
> > + int16_t residual[SUBFRAME_LEN + PITCH_ORDER - 1];
> > + const int16_t *cb_ptr;
> > + int lag = pitch_lag + subfrm.ad_cb_lag - 1;
> > +
> > + int i;
> > + int64_t sum;
> > +
> > + get_residual(residual, prev_excitation, lag);
> > +
> > + /* Select quantization table */
> > + if (cur_rate == Rate6k3 && pitch_lag < SUBFRAME_LEN - 2) {
> > + cb_ptr = adaptive_cb_gain85;
> > + } else
> > + cb_ptr = adaptive_cb_gain170;
>
> inconsistent braces
dropped
> [...]
> > +/**
> > + * Calculate pitch postfilter optimal and scaling gains.
> > + *
> > + * @param lag pitch postfilter forward/backward lag
> > + * @param ppf pitch postfilter parameters
> > + * @param cur_rate current bitrate
> > + * @param tgt_eng target energy
> > + * @param ccr cross-correlation
> > + * @param res_eng residual energy
> > + */
> > +static void comp_ppf_gains(int lag, PPFParam *ppf, Rate cur_rate,
> > + int tgt_eng, int ccr, int res_eng)
> > +{
> > + int pf_residual; /* square of postfiltered residual */
> > + int64_t temp1, temp2;
> > +
> > + ppf->index = lag;
> > +
> > + temp1 = tgt_eng * res_eng >> 1;
> > + temp2 = ccr * ccr << 1;
> > +
> > + if (temp2 > temp1) {
> > + if (ccr >= res_eng) {
> > + ppf->opt_gain = ppf_gain_weight[cur_rate];
> > + } else {
> > + ppf->opt_gain = (ccr << 15) / res_eng *
> > + ppf_gain_weight[cur_rate] >> 15;
> > + }
> > + /* pf_res^2 = tgt_eng + 2*ccr*gain + res_eng*gain^2 */
> > + temp1 = (tgt_eng << 15) + (ccr * ppf->opt_gain << 1);
> > + temp2 = (ppf->opt_gain * ppf->opt_gain >> 15) * res_eng;
> > + pf_residual = av_clipl_int32(temp1 + temp2 + (1 << 15)) >> 16;
> > +
> > + if (tgt_eng >= pf_residual << 1) {
> > + temp1 = 0x7fff;
> > + } else {
> > + temp1 = (tgt_eng << 14) / pf_residual;
> > + }
> > +
> > + /* scaling_gain = sqrt(tgt_eng/pf_res^2) */
> > + ppf->sc_gain = square_root(temp1 << 16);
> > + } else {
> > + ppf->opt_gain = 0;
> > + ppf->sc_gain = 0x7fff;
> > + }
> > +
> > + ppf->opt_gain = av_clip_int16(ppf->opt_gain * ppf->sc_gain >> 15);
>
> are you sure this requires clipping?
no idea
> > +}
> > +
> > +/**
> > + * Calculate pitch postfilter parameters.
> > + *
> > + * @param p the context
> > + * @param offset offset of the excitation vector
> > + * @param pitch_lag decoded pitch lag
> > + * @param ppf pitch postfilter parameters
> > + * @param cur_rate current bitrate
> > + */
> > +static void comp_ppf_coeff(G723_1_Context *p, int offset, int pitch_lag,
> > + PPFParam *ppf, Rate cur_rate)
> > +{
> > +
> > + int16_t scale;
> > + int i;
> > + int64_t temp1, temp2;
> > +
> > + /*
> > + * 0 - target energy
> > + * 1 - forward cross-correlation
> > + * 2 - forward residual energy
> > + * 3 - backward cross-correlation
> > + * 4 - backward residual energy
> > + */
> > + int energy[5] = {0, 0, 0, 0, 0};
> > + int16_t *buf = p->excitation + offset;
> > + int fwd_lag = autocorr_max(p, offset, &energy[1], pitch_lag,
> > + SUBFRAME_LEN, 1);
> > + int back_lag = autocorr_max(p, offset, &energy[3], pitch_lag,
> > + SUBFRAME_LEN, -1);
> > +
> > + ppf->index = 0;
> > + ppf->opt_gain = 0;
> > + ppf->sc_gain = 0x7fff;
> > +
> > + /* Case 0, Section 3.6 */
> > + if (!back_lag && !fwd_lag)
> > + return;
> > +
> > + /* Compute target energy */
> > + energy[0] = dot_product(buf, buf, SUBFRAME_LEN, 1);
> > +
> > + /* Compute forward residual energy */
> > + if (fwd_lag)
> > + energy[2] = dot_product(buf + fwd_lag, buf + fwd_lag,
> > + SUBFRAME_LEN, 1);
> > +
> > + /* Compute backward residual energy */
> > + if (back_lag)
> > + energy[4] = dot_product(buf - back_lag, buf - back_lag,
> > + SUBFRAME_LEN, 1);
> > +
> > + /* Normalize and shorten */
> > + temp1 = 0;
> > + for (i = 0; i < 5; i++)
> > + temp1 = FFMAX(energy[i], temp1);
> > +
> > + scale = normalize_bits(temp1, 1);
> > + for (i = 0; i < 5; i++)
> > + energy[i] = av_clipl_int32(energy[i] << scale) >> 16;
>
> that av_clipl_int32() is pointless
indeed, removed
> > +
> > + if (fwd_lag && !back_lag) { /* Case 1 */
> > + comp_ppf_gains(fwd_lag, ppf, cur_rate, energy[0], energy[1],
> > + energy[2]);
> > + } else if (!fwd_lag) { /* Case 2 */
> > + comp_ppf_gains(-back_lag, ppf, cur_rate, energy[0], energy[3],
> > + energy[4]);
> > + } else { /* Case 3 */
> > +
> > + /*
> > + * Select the largest of energy[1]^2/energy[2]
> > + * and energy[3]^2/energy[4]
> > + */
> > + temp1 = energy[4] * ((energy[1] * energy[1] + (1 << 14)) >> 15);
> > + temp2 = energy[2] * ((energy[3] * energy[3] + (1 << 14)) >> 15);
> > + if (temp1 >= temp2) {
> > + comp_ppf_gains(fwd_lag, ppf, cur_rate, energy[0], energy[1],
> > + energy[2]);
> > + } else {
> > + comp_ppf_gains(-back_lag, ppf, cur_rate, energy[0], energy[3],
> > + energy[4]);
> > + }
> > + }
> > +}
> > +
> > +/**
> > + * Classify frames as voiced/unvoiced.
> > + *
> > + * @param p the context
> > + * @param pitch_lag decoded pitch_lag
> > + * @param exc_eng excitation energy estimation
> > + * @param scale scaling factor of exc_eng
> > + *
> > + * @return residual interpolation index if voiced, 0 otherwise
> > + */
> > +static int comp_interp_index(G723_1_Context *p, int pitch_lag,
> > + int *exc_eng, int *scale)
> > +{
> > + int offset = PITCH_MAX + 2 * SUBFRAME_LEN;
> > + int16_t *buf = p->excitation + offset;
> > +
> > + int index, ccr, tgt_eng, best_eng, temp;
> > +
> > + *scale = scale_vector(p->excitation, FRAME_LEN + PITCH_MAX);
> > +
> > + /* Compute maximum backward cross-correlation */
> > + ccr = 0;
> > + index = autocorr_max(p, offset, &ccr, pitch_lag, SUBFRAME_LEN * 2, -1);
> > + ccr = av_clipl_int32((int64_t)ccr + (1 << 15)) >> 16;
> > +
> > + /* Compute target energy */
> > + tgt_eng = dot_product(buf, buf, SUBFRAME_LEN * 2, 1);
> > + *exc_eng = av_clipl_int32(tgt_eng + (1 << 15)) >> 16;
>
> either the av_clipl_int32() is not necessary or it won't work there. if
> the addition goes past 32-bit range it will overflow since you do not
> have a cast there like in the other 2 places in this function.
added cast
> > +
> > + if (ccr <= 0)
> > + return 0;
> > +
> > + /* Compute best energy */
> > + best_eng = dot_product(buf - index, buf - index,
> > + SUBFRAME_LEN * 2, 1);
> > + best_eng = av_clipl_int32((int64_t)best_eng + (1 << 15)) >> 16;
>
> nit: maybe make a macro for the division by 32768 with rounding and
> clipping. it seems to be used in several places.
reminds me of ARM for some reason
> > +
> > + temp = best_eng * *exc_eng >> 3;
>
> is this guaranteed not to overflow?
>
> > +
> > + if (temp < ccr * ccr) {
>
> or this?
no idea in both cases
> > + return index;
> > + } else
> > + return 0;
> > +}
> > +
> > +/**
> > + * Peform residual interpolation based on frame classification.
> > + *
> > + * @param buf decoded excitation vector
> > + * @param out output vector
> > + * @param lag decoded pitch lag
> > + * @param gain interpolated gain
> > + * @param rseed seed for random number generator
> > + */
> > +static void residual_interp(int16_t *buf, int16_t *out, int lag,
> > + int gain, int *rseed)
> > +{
> > + int i;
> > + if (lag) { /* Voiced */
> > + int16_t *vector_ptr = buf + PITCH_MAX;
> > + /* Attenuate */
> > + for (i = 0; i < lag; i++)
> > + vector_ptr[i - lag] = vector_ptr[i - lag] * 3 >> 2;
> > + av_memcpy_backptr((uint8_t*)vector_ptr, lag * sizeof(int16_t),
> > + FRAME_LEN * sizeof(int16_t));
> > + memcpy(out, vector_ptr, FRAME_LEN * sizeof(int16_t));
> > + } else { /* Unvoiced */
> > + for (i = 0; i < FRAME_LEN; i++) {
> > + *rseed = *rseed * 521 + 259;
> > + out[i] = gain * *rseed >> 15;
>
> i believe that multiplication can overflow
since it's LFG it doesn't matter
> > + }
> > + memset(buf, 0, (FRAME_LEN + PITCH_MAX) * sizeof(int16_t));
> > + }
> > +}
> > +
> > +/**
> > + * Perform IIR filtering.
> > + *
> > + * @param fir_coef FIR coefficients
> > + * @param iir_coef IIR coefficients
> > + * @param src source vector
> > + * @param dest destination vector
> > + * @param width width of the output, 16 bits(0) / 32 bits(1)
> > + */
> > +#define iir_filter(fir_coef, iir_coef, src, dest, width)\
> > +{\
> > + int m, n;\
> > + int res_shift = 16 & ~-(width);\
>
> that looks like a kind of round-about way to do 16 or 0.
>
> > + int in_shift = 16 - res_shift;\
> > +\
> > + for (m = 0; m < SUBFRAME_LEN; m++) {\
> > + int64_t filter = 0;\
> > + for (n = 1; n <= LPC_ORDER; n++) {\
> > + filter -= (fir_coef)[n - 1] * (src)[m - n] -\
> > + (iir_coef)[n - 1] * ((dest)[m - n] >> in_shift);\
> > + }\
> > +\
> > + (dest)[m] = av_clipl_int32(((src)[m] << 16) + (filter << 3) +\
> > + (1 << 15)) >> res_shift;\
> > + }\
> > +}
>
> I don't see any need for this to be a macro. An inline function should
> do just fine and would be more readable.
indeed, done
> [...]
> > +static int g723_1_decode_frame(AVCodecContext *avctx, void *data,
> > + int *got_frame_ptr, AVPacket *avpkt)
> > +{
> > + G723_1_Context *p = avctx->priv_data;
> > + const uint8_t *buf = avpkt->data;
> > + int buf_size = avpkt->size;
> > + int dec_mode = buf[0] & 3;
> > +
> > + PPFParam ppf[SUBFRAMES];
> > + int16_t cur_lsp[LPC_ORDER];
> > + int16_t lpc[SUBFRAMES * LPC_ORDER];
> > + int16_t acb_vector[SUBFRAME_LEN];
> > + int16_t *vector_ptr;
> > + int bad_frame = 0, i, j, ret;
> > +
> > + if (!buf_size || buf_size < frame_size[dec_mode]) {
>
> would the second condition also cover the first?
yes, dropped
> > + *got_frame_ptr = 0;
> > + return buf_size;
> > + }
>
> this should print a message and return an error instead of silently
> consuming the too-small packet
added
> > +
> > + if (unpack_bitstream(p, buf, buf_size) < 0) {
> > + bad_frame = 1;
> > + p->cur_frame_type = p->past_frame_type == ActiveFrame ?
> > + ActiveFrame : UntransmittedFrame;
> > + }
> > +
> > + p->frame.nb_samples = FRAME_LEN;
> > + if ((ret = avctx->get_buffer(avctx, &p->frame)) < 0) {
> > + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> > + return ret;
> > + }
> > +
> > + if(p->cur_frame_type == ActiveFrame) {
>
> space after "if"
added
> > + if (!bad_frame) {
> > + p->erased_frames = 0;
> > + } else if(p->erased_frames != 3)
> > + p->erased_frames++;
>
> inconsistent braces
dropped
> [...]
> > +AVCodec ff_g723_1_decoder = {
> > + .name = "g723_1",
> > + .type = AVMEDIA_TYPE_AUDIO,
> > + .id = CODEC_ID_G723_1,
> > + .priv_data_size = sizeof(G723_1_Context),
> > + .init = g723_1_decode_init,
> > + .decode = g723_1_decode_frame,
> > + .long_name = NULL_IF_CONFIG_SMALL("G.723.1"),
> > + .capabilities = CODEC_CAP_SUBFRAMES,
>
> So the packets can contain multiple frames? It looks like a parser would
> be very simple. Should we do that?
It's an overkill - _maximum_ packet size here is 24 bytes.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
