[Ffmpeg-devel] [PATCH] qcelp codec supporrt

[Moriyoshi Koizumi]

Hi,

On Tue, 2006-12-12 at 21:36 +0100, Michael Niedermayer wrote:
> 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
> 

Aren't they aligned to word boundaries by the compiler?

> > +            *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

Is it okay to use the function that is part of C99? 

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

I put it in the for loop because I didn't realize how the AVParser
handles it and hands it to the decoder. I'll look into it.

> [...]
> 
> > +    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

I didn't find any docs for them except the specification. So part of
this are actually copy'n'paste stuff. As far as I read the source,
probably target_snr_thr and reduced_rate_flag (used to decide which band
rate to use), unvoiced_off (to enable / disable unvoiced mode) should be
user selectable at least.


> 
> > +    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)

Right, but the size of the resulting file was considerably decreased
when I put as large packets as feasible. Would there be any room for
optimisation without breaking the rule?

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

I appended it because I thought signed to unsigned conversion
between two values of the different storage size is not
defined in the C spefication.

-- 
Moriyoshi Koizumi (?? ??)
GREE, Inc.