[FFmpeg-devel] [PATCH] HE-AAC v2 decoder try 2

[Reimar Döffinger]

On Tue, Jun 15, 2010 at 05:54:03PM -0400, Alex Converse wrote:
> @@ -471,6 +473,9 @@ static int decode_audio_specific_config(AACContext *ac, void *data,
>          av_log(ac->avctx, AV_LOG_ERROR, "invalid sampling rate index %d\n", ac->m4ac.sampling_index);
>          return -1;
>      }
> +    if (ac->m4ac.sbr == 1 && ac->m4ac.ps) {
> +        ac->m4ac.ps = 1;
> +    }

Pointless {} and maybe for readability and consistency with other code parts it
would be better to write ac->m4ac.ps == -1 ?

> @@ -1667,6 +1672,10 @@ static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt,
>              av_log(ac->avctx, AV_LOG_ERROR, "Implicit SBR was found with a first occurrence after the first frame.\n");
>              skip_bits_long(gb, 8 * cnt - 4);
>              return res;
> +        } else if (ac->m4ac.sbr == -1 && ac->output_configured < OC_LOCKED && ac->m4ac.ps == -1 && ac->avctx->channels == 1) {

Hmm... Does the value of sbr really matter here? Since sbr is set unconditionally in the lese part...

> +        if (num_bits_left < 0) {
> +            av_log(ac->avctx, AV_LOG_ERROR, "num_bits_left %d\n", num_bits_left);
> +            abort();
> +        }

abort is quite rare in FFmpeg, I think this should be documented why it is
acceptable to use it.

> @@ -1166,7 +1175,7 @@ static void sbr_qmf_analysis(DSPContext *dsp, FFTContext *mdct, const float *in,
>   * (14496-3 sp04 p206)
>   */
>  static void sbr_qmf_synthesis(DSPContext *dsp, FFTContext *mdct,
> -                              float *out, float X[2][32][64],
> +                              float *out, float X[2][38][64],
>                                float mdct_buf[2][64],
>                                float *v0, int *v_off, const unsigned int div,
>                                float bias, float scale)
> @@ -1402,7 +1411,7 @@ static int sbr_hf_gen(AACContext *ac, SpectralBandReplication *sbr,
>  }
>  
>  /// Generate the subband filtered lowband
> -static int sbr_x_gen(SpectralBandReplication *sbr, float X[2][32][64],
> +static int sbr_x_gen(SpectralBandReplication *sbr, float X[2][38][64],
>                       const float X_low[32][40][2], const float Y[2][38][64][2],
>                       int ch)
>  {
> @@ -1424,7 +1433,7 @@ static int sbr_x_gen(SpectralBandReplication *sbr, float X[2][32][64],
>      }
>  
>      for (k = 0; k < sbr->kx[1]; k++) {
> -        for (i = i_Temp; i < i_f; i++) {
> +        for (i = i_Temp; i < 38; i++) {
>              X[0][i][k] = X_low[k][i + ENVELOPE_ADJUSTMENT_OFFSET][0];
>              X[1][i][k] = X_low[k][i + ENVELOPE_ADJUSTMENT_OFFSET][1];
>          }

Could it make sense to give this a name instead of using a "magic" 38?

> +#if 0
> +            h11r = 0;
> +            h12r = 1;
> +            h21r = 1;
> +            h22r = 0;
> +            h11i = h12i = h21i = h22i = 0;
> +#endif

Hm?

> +void write_float_3d_array (const void *p, int b, int c, int d)
> +{
> +    int i;
> +    for (i = 0; i < b; i++) {
> +        printf("{\n");
> +        write_float_2d_array(p, c, d);
> +        printf("},\n");
> +        p += c * d * sizeof(float);
> +    }

Use a extra local float* variable, arithmetic on void * isn't valid.