[FFmpeg-devel] [PATCH] Implement AAC Long Term Prediction (LTP) decoding module

[Diego Biurrun]

On Sun, Jan 30, 2011 at 12:33:37AM +0900, Young Han Lee wrote:
> 
> Based on SoC repo., the ltp module is implemented at the AAC decoder.
> This is my first patch.
> Any comments are welcome.

You will soon here from the local AAC experts, just some small comments,
mostly style nits, from me.  You should follow the (K&R) style of the file,
4 spaces indentation, no tabs.

> --- a/libavcodec/aacdec.c
> +++ b/libavcodec/aacdec.c
> @@ -629,6 +631,25 @@ static int decode_prediction(AACContext *ac, IndividualChannelStream *ics,
>  
>  /**
> + * Decode Long Term Prediction data; reference: table 4.xx.
> + *
> + * @return  void
> + */

void functions return nothing, drop the @return.

> +static void decode_ltp(AACContext * ac, LongTermPrediction * ltp, GetBitContext * gb, uint8_t max_sfb) {

Leave out the space between pointer * and variable name, break this long
line and place the { on the next line for function declarations, i.e.

static void decode_ltp(AACContext *ac, LongTermPrediction *ltp,
                       GetBitContext *gb, uint8_t max_sfb)
{

The pointer * should be 
> +	int sfb;
> +	if (ac->m4ac.object_type == AOT_ER_AAC_LD) {
> +		av_log(ac->avctx, AV_LOG_ERROR, "LTP is not supported in ER AAC LD .\n");
> +	} else {
> +		ltp->lag = get_bits(gb, 11);
> +		ltp->coef = ltp_coef[get_bits(gb, 3)] * ac->sf_scale;
> +		for (sfb = 0; sfb < FFMIN(max_sfb, MAX_LTP_LONG_SFB); sfb++) {
> +			ltp->used[sfb] = get_bits1(gb);
> +		}
> +	}
> +}

Indent with 4 spaces, not with tabs, extra good karma for aligning the
'=' in the struct member assignments.

> @@ -1682,14 +1707,89 @@ static void apply_tns(float coef[1024], TemporalNoiseShaping *tns,
>  
> +	if (sce->ics.window_sequence[0] != EIGHT_SHORT_SEQUENCE) {
> +		float x_est[2048], X_est[1024];
> +		int16_t num_samples = 2048;
> +		if ( ltp->lag < 1024)

Drop the space after the '('.

> +		if(sce->tns.present)

if (

> @@ -1855,6 +1967,14 @@ static void spectral_to_sample(AACContext *ac)
>              if (che) {
>                  if (type <= TYPE_CPE)
>                      apply_channel_coupling(ac, che, type, i, BEFORE_TNS, apply_dependent_coupling);
> +                if (che->ch[0].ics.predictor_present) {
> +                    if (ac->m4ac.object_type == AOT_AAC_LTP) {
> +                        if(che->ch[0].ics.ltp.present)
> +                            apply_ltp(ac, &che->ch[0]);
> +                        if(type == TYPE_CPE && che->ch[1].ics.ltp.present)
> +                            apply_ltp(ac, &che->ch[1]);

if (

> --- a/libavcodec/aacdectab.h
> +++ b/libavcodec/aacdectab.h
> @@ -35,6 +35,15 @@
>  
> +/* @name ltp_coef
> + * Table of the LTP coefficient (multiplied by 2)
> + * @{
> + */
> +static const float ltp_coef[8] = {
> +	 1.141658,    1.393232,    1.626008,    1.822608,
> +	 1.969800,    2.135788,    2.2389202,   2.739066,
> +};

The @{ looks wrong without a closing @}.  It would also be interesting
to know why you multiply by 2, not just that you have done it.

Diego