[FFmpeg-devel] [PATCH] GSM 6.10 audio decoder

[Ronald S. Bultje]

Hi,

On Sat, Jul 3, 2010 at 7:03 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> I fear I may have gone a bit too far when removing clipping protections,

Looks fine to me, I think...

> As for the purpose: I want to throw out MPlayer's "native" variant
> of this, which is basically libgsm just with a bit more of those horrible
> old-style function declarations.

Great!

> +#define GSM_BLOCK_SIZE 33
> +#define GSM_MS_BLOCK_SIZE 65
> +#define GSM_FRAME_SIZE 160

Missing doxy. E.g. are they in bits, bytes, what are they, etc. (see
other speech codecs as example).

> +static int16_t dequant_tab[64][8] = {
[cut]

gsmdata.h, missing const.

> +static void apcm_dequant_add(GetBitContext *gb, int16_t *dst)
> +{
> +    int i;
> +    int maxidx = get_bits(gb, 6);
> +    int16_t *tab = dequant_tab[maxidx];

const.

> +static int gsm_mult(int a, int b)
> +{
> +    return (a * b + (1 << 14)) >> 15;
> +}

Should probably be inline, just in case some compiler is silly.

> +static inline int decode_log_area(int coded, int factor, int offset)
> +{
> +    coded <<= 10;
> +    coded -= offset;
> +    return gsm_mult(coded, factor) << 1;
> +}

E.g. here that will save several instructions.

> +static int get_rrp(int filtered)
> +{
> +    int abs = filtered;
> +    if (abs < 0) abs = -abs;
> +    if (abs < 11059) abs <<= 1;
> +    else if (abs < 20070) abs += 11059;
> +    else abs = (abs >> 2) + 26112;
> +    return filtered < 0 ? -abs : abs;
> +}

FFABS() as already pointed out, vertical align would help a lot also.
What's "rrp"? Note also that you're doing "filtered<0" twice here, you
could do both of that at once (if gcc doesn't already, check
disassembly), as in:

if (filtered < 0) {
    abs  = -filtered;
    sign = -1;
} else {
    abs  =  filtered;
    sign =  1;
}

if (abs < 11059)      abs <<= 1;
else if (abs < 20070) abs += 11059;
else                  abs = (abs >> 2) + 26112

return sign * abs;

> +
> +static int filter_value(int in, int rrp[8], int v[9])
> +{
> +    int i;
> +    for (i = 7; i >= 0; i--) {
> +        in -= gsm_mult(rrp[i], v[i]);
> +        v[i + 1] = v[i] + gsm_mult(rrp[i], in);
> +    }
> +    v[0] = in;
> +    return in;
> +}

Is this similar to the inner loop of ff_celp_lp_synthesis_filter()?
Can we generalize that to decrease binary size without significantly
hurting performance?

> +static void short_term_synth(GSMContext *ctx, int16_t *dst, const int16_t *src)
> +{
> +    int i;
> +    int rrp[8];
> +    int *lar = ctx->lar[ctx->lar_idx];
> +    int *lar_prev = ctx->lar[ctx->lar_idx ^ 1];
> +    for (i = 0; i < 8; i++)
> +        rrp[i] = get_rrp((lar_prev[i] >> 2) + (lar_prev[i] >> 1) + (lar[i] >> 2));
> +    for (i = 0; i < 13; i++)
> +        dst[i] = filter_value(src[i], rrp, ctx->v);
[cut]

Note how ff_celp_lp_synthesis_filter() does this loop in the actual
function itself. You might want to follow a similar design, this is
what other audio codecs do also (e.g. ra144.c).

> +static int gsm_decode_block(AVCodecContext *avctx, int16_t *samples,
> +                            GetBitContext *gb)
[..]
> +    for (i = 0; i < 4; i++) {
> +        int lag      = get_bits(gb, 7);
> +        int gain_idx = get_bits(gb, 2);
> +        int offset   = get_bits(gb, 2);
> +        lag = av_clip(lag, 40, 120);

lag = av_clip(get_bits(), .., ..);

> +    memcpy(ctx->ref_buf, ctx->ref_buf + 160, 120 * sizeof(*ctx->ref_buf));
> +    short_term_synth(ctx, samples, ctx->ref_buf + 120);
> +    ctx->msr = postprocess(samples, ctx->msr);

memcpy() is slow, can you change postprocess to take ref_buf as input
and output into samples? That's what all other voice codecs do.

> +    switch(avctx->codec_id) {
> +    case CODEC_ID_GSM:
> +        if (get_bits(&gb, 4) != 0xd)
> +            av_log(avctx, AV_LOG_WARNING, "Missing GSM magic!\n");
> +        res = gsm_decode_block(avctx, samples, &gb);
> +        if (res < 0)
> +            return res;
> +        *data_size = 2*GSM_FRAME_SIZE;
> +        break;
> +    case CODEC_ID_GSM_MS:
> +        res = gsm_decode_block(avctx, samples, &gb);
> +        if (res < 0)
> +            return res;
> +        res = gsm_decode_block(avctx, samples + GSM_FRAME_SIZE, &gb);
> +        if (res < 0)
> +            return res;

This can probably be written smaller.

> +        *data_size = 2*2*GSM_FRAME_SIZE;
> +    }
> +    *data_size = frame_bytes;

Isn't that a little pointless?

> +AVCodec gsm_decoder = {
> +    "gsm",
> +    AVMEDIA_TYPE_AUDIO,
> +    CODEC_ID_GSM,
> +    sizeof(GSMContext),
> +    gsm_init,
> +    NULL,
> +    NULL,
> +    gsm_decode_frame,
> +    .long_name = NULL_IF_CONFIG_SMALL("GSM"),
> +};
> +
> +AVCodec gsm_ms_decoder = {
> +    "gsm_ms",
> +    AVMEDIA_TYPE_AUDIO,
> +    CODEC_ID_GSM_MS,
> +    sizeof(GSMContext),
> +    gsm_init,
> +    NULL,
> +    NULL,
> +    gsm_decode_frame,
> +    .long_name = NULL_IF_CONFIG_SMALL("GSM Microsoft variant"),
> +};

You could use a macro here to decrease source code size.

Ronald