[Ffmpeg-devel] [PATCH] qcelp codec supporrt

[Michael Niedermayer]

Hi

On Mon, Dec 11, 2006 at 12:21:58PM +0900, Moriyoshi Koizumi wrote:
> Hello,
> 
> I implemented avcodec stub code that glues ffmpeg with the reference
> implementation of QCELP which is provided in the 3GPP2 project page.
> 
> Any comments are welcome.

[...]

> +static const int frame_size_map[16] = {
> +     0,  3,  7, 16, 34,  7, -1, -1,
> +    -1, -1, -1, -1, -1, -1,  0, -1,
> +};

this should fit in int8_t which would save a few bytes


[...]

> +static int qcelp_decode_frame(AVCodecContext * avctx,
> +            void *data, int *data_size,
> +            uint8_t * buf, int buf_size)
> +{
> +    QCELPContext *s = (QCELPContext *)avctx->priv_data;
> +    const uint8_t *p, *end;
> +    short *q;
> +    float samples[FSIZE];
> +    struct PACKET pkt;
> +
> +    memset(&pkt, 0, sizeof(pkt));
> +
> +    p = buf;
> +    q = (short *)data;
> +
> +    for (end = buf + buf_size; p < end; ) {
> +        int i, noctets;
> +        const uint8_t *lim;
> +
> +        noctets = frame_size_map[(pkt.data[0] = *p++) & 0x0f];
> +        if (noctets < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "decoder error (%d)\n", pkt.data[0]);
> +            return -1;
> +        }
> +
> +        if (p + noctets > end) {
> +            break;
> +        }
> +
> +        for (i = 0, lim = p + (noctets & ~1); p < lim; p += 2) {
> +            pkt.data[++i] = BE_16(p);
> +        }
> +
> +        if ((noctets & 1)) {
> +            pkt.data[++i] = *p++;
> +        }
> +
> +        decoder(samples, &pkt, &s->control, &s->decoder);
> +
> +        for (i = 0; i < FSIZE; i++) {
> +            *q++ = FFMIN(32768, FFMAX(-32768, samples[i] * 4));

16bit signed is -32768 .. 32767 not 32768
also this could use clip(lrintf()) instead of MIN+MAX


the packet spliting should be done in an AVParser so that that decoder always
gets exactly one packet each time


[...]
> +static int qcelp_encode_init(AVCodecContext *avctx)
> +{
> +    QCELPContext *s = (QCELPContext *)avctx->priv_data;
> +    static int rate_frac_map[] = {

this should be const static int


[...]

> +    s->control.min_rate = EIGHTH;
> +    s->control.max_rate = rate_frac_map[rate_frac];
> +    s->control.avg_rate = 9.0;
> +    s->control.target_snr_thr = 10.0;
> +    s->control.pf_flag   = PF_ON;
> +    s->control.cb_out    = NO;
> +    s->control.pitch_out = NO;
> +    s->control.print_packets = NO;
> +    s->control.output_encoder_speech = NO;
> +    s->control.form_res_out = NO;
> +    s->control.reduced_rate_flag = NO;
> +    s->control.unvoiced_off = YES;
> +    s->control.pitch_post = YES;
> +    s->control.per_wght = PERCEPT_WGHT_FACTOR;
> +    s->control.target_after_out = NO;
> +    s->control.fractional_pitch = YES;

what do all these do? are there any docs for them? iam asking because
maybe some should be user selectable


> +    avctx->extradata_size = 0;

unneeded 0 should be default


> +    avctx->frame_size = FSIZE * 10; /* hmm.. */

each AVPacket should correspond to a single packet unless the packets
are all the same size and very small (like for PCM)


[...]
> +            *p++ = (pkt.data[i] >> 8) & 0xff;
> +            *p++ = pkt.data[i] & 0xff;

the & 0xff isnt needed

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali