[FFmpeg-devel] [PATCH] HE-AAC v1 decoder

Robert Swain robert.swain
Wed Jan 27 13:24:35 CET 2010


On 27/01/10 11:53, Alexander Strange wrote:
>
> On Jan 26, 2010, at 10:10 PM, Alex Converse wrote:

[...]

> Some comments:
>
>> @@ -1815,7 +1814,7 @@ static void apply_independent_coupling(AACContext *ac,
>>       const float *src = cce->ch[0].ret;
>>       float *dest = target->ret;
>>
>> -    for (i = 0; i<  1024; i++)
>> +    for (i = 0; i<  2048; i++)
>>           dest[i] += gain * (src[i] - bias);
>>   }
>>
> Looks like a SIMD opportunity.

Indeed and there are probably many more. This one isn't SBR-specific 
though. :)

>> +    if (sbr->sample_rate == 16000) {
>> +        sbr_offset_ptr = sbr_offset[0];
>> +    } else if (sbr->sample_rate == 22050) {
>> +        sbr_offset_ptr = sbr_offset[1];
>> +    } else if (sbr->sample_rate == 24000) {
>> +        sbr_offset_ptr = sbr_offset[2];
>> +    } else if (sbr->sample_rate == 32000) {
>> +        sbr_offset_ptr = sbr_offset[3];
>> +    } else if ((sbr->sample_rate>= 44100)&&
>> +               (sbr->sample_rate<= 64000)) {
>> +        sbr_offset_ptr = sbr_offset[4];
>> +    } else if (sbr->sample_rate>  64000) {
>> +        sbr_offset_ptr = sbr_offset[5];
>> +    } else {
>> +        av_log(ac->avccontext, AV_LOG_ERROR,
>> +               "Unsupported sample rate for SBR: %d\n", sbr->sample_rate);
>> +        return -1;
>> +    }
>
> Kind of long. I'd use a switch, except for the arbitrary samplerates allowed after 44.1k (why only there?).

Because that's what the spec says. :) When writing it I thought a switch 
and an if would be uglier than a longer if chain.

>> +        if (sbr->k[2] / (float)sbr->k[0]>  2.2449) {
>> +            two_regions = 1;
>> +            sbr->k[1] = sbr->k[0]<<  1;
>> +        } else {
>> +            two_regions = 0;
>> +            sbr->k[1] = sbr->k[2];
>> +        }
>
> Here it should be 2.2449f. What's 'k'?

They're subbands that are significant for later processes. (See below.)

>> +/// High Frequency Generation - Patch Construction (14496-3 sp04 p216 fig. 4.46)
>> +static int sbr_hf_calc_npatches(AACContext *ac, SpectralBandReplication *sbr)
>> +{
>> +    int i, k, sb = 0;
>> +    int msb = sbr->k[0];
>> +    int usb = sbr->k[4];
>> +    int goal_sb = lroundf((1<<  11) * 1000 / (float)sbr->sample_rate);
>
> What do those names mean?

They're in the spec like that. The 'sb' part means subband in this case. 
k0 is the first QMF subband in the f_Master table and k[4] is k_x which 
is the first subband in the SBR range (the first subband in f_TableHigh).

To take a wild guess, maybe 'master subband' and 'upper subband', though 
the latter seems particularly tenuous to me.

>> +static inline int in_table(void *table, int last_el, int el_size, void *needle)
>> +{
>> +    int i;
>> +    uint8_t *table_ptr = table; // avoids a warning with void * ptr arith
>> +    for (i = 0; i<= last_el; i++, table_ptr += el_size)
>> +        if (!memcmp(table_ptr, needle, el_size))
>> +            return 1;
>> +    return 0;
>> +}
>
> I think the comment is superfluous.
> Also, this is only used with sizeof(int16_t).

I wrote it to be pretty generic as I thought maybe it could be used 
elsewhere.

>> --- /dev/null
>> +++ b/libavcodec/aacsbr_internal.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * AAC Spectral Band Replication function delcarations
>> + * Copyright (c) 2008-2009 Robert Swain ( rob opendot cl )
>
> declarations (and it's 2010)

I only worked on it 'til last year. Alex worked on it last year and into 
this year. Maybe nothing was changed in that file since last year.

Regards,
Rob



More information about the ffmpeg-devel mailing list