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

[Martin Storsjö]

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.

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

> > +    if (avctx->sample_rate != 8000 && avctx->sample_rate != 16000) {
> > +        av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate [%d] only 8KHz "
> > +                                    "and 16KHz are supported.\n",
> > +                                    avctx->sample_rate);
> > +        return AVERROR_INVALIDDATA;
> > +    }
> 
> this looks lame, the code surely works too with 22khz or 44khz or anything
> else, its just a stream of integers that are being compressed or decompressed
> the samplerate is not essential

Yes, that's true. The codec itself is only specified for 16 kHz, but 
there's nothing stopping the user from using it on any data, as you said. 
This restriction was earlier used to be able to decode only the lower 
subband if 8 kHz was chosen, but I'm removing that function for now - if 
it is desired, I guess it should be signalled in some other way.

Updated patch attached.

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initial-G.722-decoder-patch.patch
Type: text/x-diff
Size: 14526 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100908/8e8c2aa6/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-G.722-muxer-demuxer.patch
Type: text/x-diff
Size: 2874 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100908/8e8c2aa6/attachment-0001.patch>