[FFmpeg-devel] [PATCH] G.722 decoder

[Michael Niedermayer]

On Wed, Sep 08, 2010 at 04:38:36PM +0300, Martin Storsj? wrote:
> On Wed, 8 Sep 2010, Michael Niedermayer wrote:
> 
> > On Wed, Sep 08, 2010 at 09:18:21AM +0300, Martin Storsj? wrote:
> > > +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff)
> > > +{
> > > +    int sg[2], limit, i, cur_qtzd_reconst;
> > > +
> > > +    const int cur_part_reconst = band->s_zero + cur_diff < 0;
> > > +
> > > +    sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]];
> > > +    sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]];
> > > +    band->part_reconst_mem[1] = band->part_reconst_mem[0];
> > > +    band->part_reconst_mem[0] = cur_part_reconst;
> > > +
> > > +    band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) +
> > > +                                (sg[1] << 7) + (band->pole_mem[1] * 127 >> 7), -12288, 12288);
> > > +
> > > +    limit = 15360 - band->pole_mem[1];
> > > +    band->pole_mem[0] = av_clip(-192 * sg[0] + (band->pole_mem[0] * 255 >> 8), -limit, limit);
> > > +
> > > +
> > > +    if (cur_diff) {
> > > +        for (i = 0; i < 6; i++)
> > > +            band->zero_mem[i] = ((band->zero_mem[i]*255) >> 8) +
> > > +                                ((band->diff_mem[i]^cur_diff) < 0 ? -128 : 128);
> > > +    } else
> > 
> > > +        for (i = 0; i < 6; i++)
> > > +            band->zero_mem[i] = (band->zero_mem[i]*255) >> 8;
> > 
> > > +
> > > +    for (i = 5; i > 0; i--)
> > > +        band->diff_mem[i] = band->diff_mem[i-1];
> > > +    band->diff_mem[0] = av_clip_int16(cur_diff << 1);
> > > +
> > > +    band->s_zero = 0;
> > > +    for (i = 5; i >= 0; i--)
> > > +        band->s_zero += (band->zero_mem[i]*band->diff_mem[i]) >> 15;
> > > +
> > > +
> > > +    cur_qtzd_reconst = av_clip_int16((band->s_predictor + cur_diff) << 1);
> > > +    band->s_predictor = av_clip_int16(band->s_zero +
> > > +                                      (band->pole_mem[0] * cur_qtzd_reconst >> 15) +
> > > +                                      (band->pole_mem[1] * band->prev_qtzd_reconst >> 15));
> > > +    band->prev_qtzd_reconst = cur_qtzd_reconst;
> > > +}
> > 
> > somehow iam feeling this function should return s_predictor and not void
> 
> Yes, that would perhaps be logical, however, in this codec, the updated 
> predictor value isn't used until decoding the next sample, due to the 
> scalable bitrate design (where different inv quant tables are used for 
> decoding the actual samples, but the one same lower precision one always 
> is used for updating the predictor internally, regardless which way it is 
> decoded). So making this return the predictor wouldn't really help 
> anything in this implementation.
> 
> > > +static int inline linear_scale_factor(const int log_factor, int shift)
> > > +{
> > > +    const int wd1 = inv_log2_table[(log_factor >> 6) & 31];
> > > +    shift -= log_factor >> 11;
> > > +    return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2;
> > > +}
> > 
> > the shift parameter can be droped and instead a constant added to the
> > log_factor param for each call
> 
> Yes, dropped this parameter.
>

> > also the <<2 can be removed, scale_factor be scaled down by 4 thus and a
> > few shifts readjusted in uses of it
> 
> Hmm, I'm not sure I'm following you here. Do you mean that I should 
> premultiply inv_log2_table by 4 and skip the final right shift? Or just 
> add/subtract 2 from the 'shift' variable? I tried that (the former, but 
> the latter would have the same effect), but it doesn't give bitexact 
> output, since the codec relies on truncating the values when doing the 
> right shift.

ive meant making scale_factor= scale_factor/4 and simplifying all code that
reads or writes it. This seems possible, am i missing something?


> 
> > > +static av_cold int g722_init(AVCodecContext * avctx)
> > > +{
> > > +    G722Context *c = avctx->priv_data;
> > > +
> > > +    if (avctx->channels != 1) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Only mono tracks are allowed.\n");
> > > +        return AVERROR_INVALIDDATA;
> > > +    }
> > > +    avctx->sample_fmt = SAMPLE_FMT_S16;
> > > +
> > > +    switch (avctx->bit_rate) {
> > > +    case 64000:
> > > +    case 56000:
> > > +    case 48000:
> > > +        c->bits_per_sample = avctx->bit_rate/8000;
> > > +        break;
> > 
> > theres a problem here
> > bitrate is the bitrate of the bitstream in libavcodec
> > thus a lower bitrate stream + trash bits has 64000 too you cant distinguish
> > them like this
> 
> Yes, this is a problem. Do you have any other suggestion for how to signal 
> the lower bitrate variants? Would AVCodecContext->bits_per_coded_sample 
> perhaps be suitable for this?

maybe

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100908/bd5b660b/attachment.pgp>