On 19 July 2011 15:39, Diego Biurrun <[email protected]> wrote:
> On Tue, Jul 19, 2011 at 08:48:07AM +0200, Rob wrote:
>> On 15 July 2011 23:48, Diego Biurrun <[email protected]> wrote:
>> > Somebody please doublecheck that I was not overeager with the unused
>> > functions in those *celp*.c files, possibly they were intended to
>> > fulfill some sort of purpose at some future point in time.
>>
>> Have you spoken to Vladimir Voroshilov about this? Maybe the changes
>> needed to make it both compile and work are not large.
>
> Yes, if you look through the ml, you fill find a short thread:
>
> Subject: [libav-devel] vestigial G.729 decoder
>
> Let me quickly quote my question and Vladimir's reply here:
>
> > > We have the beginnings of a G.729 decoder scattered over
> > >
> > > libavcodec/g729dec.c
> > > libavcodec/g729data.h
> > >
> > > and a bunch of #ifdef blocks in various other files.
> > >
> > > This was added by Vladimir Voroshilov in 2006, but never finished.
> > > It did not even compile back then, much less today.
> > >
> > > Does this thing have any value or should we remove it?
> >
> > If noone wants to finish g729, i'm ok with removing those parts, since
> > i don't have neither free time nor enough skills to done this work in
> > visible future.
Thanks for the quote. Just wanted to be sure.
>> If you insist on ripping it out (the code looks good to me) then
>> perhaps it could be moved to some holding place rather than just
>> removed.
>
> I was going to reference it on the "interesting patches" section of the
> multimedia wiki, is that enough for you?
Yes, as long as it's noted somewhere. It would be helpful if someone
could shout up about it if anyone ever comes along who is interested
in it too.
>> > --- a/libavcodec/acelp_pitch_delay.c
>> > +++ b/libavcodec/acelp_pitch_delay.c
>> > @@ -88,39 +88,6 @@ void ff_acelp_update_past_gain(
>> > quant_energy[0] = (6165 * ((ff_log2(gain_corr_factor) >> 2) - (13
>> > << 13))) >> 13;
>> > }
>> >
>> > -int16_t ff_acelp_decode_gain_code(
>> > - DSPContext *dsp,
>> > - int gain_corr_factor,
>> > - const int16_t* fc_v,
>> > - int mr_energy,
>> > - const int16_t* quant_energy,
>> > - const int16_t* ma_prediction_coeff,
>> > - int subframe_size,
>> > - int ma_pred_order)
>> > -{
>> > - int i;
>> > -
>> > - mr_energy <<= 10;
>> > -
>> > - for(i=0; i<ma_pred_order; i++)
>> > - mr_energy += quant_energy[i] * ma_prediction_coeff[i];
>> > -
>> > -#ifdef G729_BITEXACT
>> > - mr_energy += (((-6165LL * ff_log2(dsp->scalarproduct_int16(fc_v,
>> > fc_v, subframe_size, 0))) >> 3) & ~0x3ff);
>> > -
>> > - mr_energy = (5439 * (mr_energy >> 15)) >> 8; // (0.15) =
>> > (0.15) * (7.23)
>> > -
>> > - return bidir_sal(
>> > - ((ff_exp2(mr_energy & 0x7fff) + 16) >> 5) *
>> > (gain_corr_factor >> 1),
>> > - (mr_energy >> 15) - 25
>> > - );
>> > -#else
>> > - mr_energy = gain_corr_factor * exp(M_LN10 / (20 << 23) * mr_energy) /
>> > - sqrt(dsp->scalarproduct_int16(fc_v, fc_v, subframe_size,
>> > 0));
>> > - return mr_energy >> 12;
>> > -#endif
>> > -}
>>
>> Hmm, this function looks very familiar to me for AMR though my
>> implementation was float, sipro appears to use a floating point
>> version of this function and I know it is a core function of ACELP
>> codecs so perhaps it is good to keep around.
>>
>> We were aiming to build up a collection of the common ACELP functions
>> to reuse to quickly write other ACELP decoders. If removed it would be
>> in the history but it would be difficult to know about or find for
>> someone who didn't know it had been there.
>
> I'll take that as a vote to keep it then.
Yes.
>> > - * @remark The routine is used in G.729 and AMR (all modes).
>> > - */
>> > -int16_t ff_acelp_decode_gain_code(
>> > - DSPContext *dsp,
>> > - int gain_corr_factor,
>> > - const int16_t* fc_v,
>> > - int mr_energy,
>> > - const int16_t* quant_energy,
>> > - const int16_t* ma_prediction_coeff,
>> > - int subframe_size,
>> > - int max_pred_order);
>>
>> Because of the remark, I would suggest leaving this where it is.
>
> But it is not used in AMR, at least currently...
No, but implementing a fixed point AMR Narrowband decoder is perhaps
something of interest to someone so it gets a vote to keep it for the
same reason as above.
Regards,
Rob
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
