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

[Alex Converse]

On Wed, Jun 16, 2010 at 1:38 AM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> 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 ?
>

Fixed

>> @@ -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...
>

Fixed

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

Fixed (old debug code)

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

It's the maximum value of numTimeSlots*RATE plus 6 for the PS
filterbank. Any thoughts on a name?

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

old debug code. Fixed

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

Fixed.