[FFmpeg-devel] [PATCH] AAC decoder

[Robert Swain]

2008/4/2 Michael Niedermayer <michaelni at gmx.at>:
> On Tue, Apr 01, 2008 at 04:56:48PM +0200, Andreas ?man wrote:
>> Andreas ?man wrote:

[...]

>> +/**
>> + * Decode scale_factor_data
>> + * reference: Table 4.47
>> + */
>> +static int scale_factor_data(AACContext * ac, GetBitContext * gb, float mix_gain, unsigned int global_gain, ics_struct * ics, const int cb[][64], float sf[][64]) {
>
> Maybe rename the function to what the comment says
>
>
>> +    int g, i;
>> +    unsigned int intensity = 100; // normalization for intensity_tab lookup table
>> +    int noise = global_gain - 90;
>> +    int noise_flag = 1;
>> +    ics->intensity_present = 0;
>> +    for (g = 0; g < ics->num_window_groups; g++) {
>> +        for (i = 0; i < ics->max_sfb; i++) {
>> +            if (cb[g][i] == ZERO_HCB) {
>> +                sf[g][i] = 0.;
>
>> +            } else if (cb[g][i] == INTENSITY_HCB || cb[g][i] == INTENSITY_HCB2) {
>> +                ics->intensity_present = 1;
>> +                intensity += get_vlc2(gb, ac->mainvlc.table, 7, 3) - 60;
>> +                if(intensity > 255) {
>> +                    av_log(ac->avccontext, AV_LOG_ERROR,
>> +                           "Intensity (%d) out of range", intensity);
>> +                    return -1;
>> +                }
>> +                sf[g][i] = ac->intensity_tab[intensity];
>> +            } else if (cb[g][i] == NOISE_HCB) {
>> +                if (noise_flag) {
>> +                    noise_flag = 0;
>> +                    noise += get_bits(gb, 9) - 256;
>> +                } else {
>> +                    noise += get_vlc2(gb, ac->mainvlc.table, 7, 3) - 60;
>> +                }
>> +                sf[g][i] = pow(2.0, 0.25 * noise) * ac->sf_scale;
>> +            } else {
>> +                global_gain += get_vlc2(gb, ac->mainvlc.table, 7, 3) - 60;
>> +                if(global_gain > 255) {
>> +                    av_log(ac->avccontext, AV_LOG_ERROR,
>> +                           "Global gain (%d) out of range", global_gain);
>> +                    return -1;
>> +                }
>> +                sf[g][i] = ac->pow2sf_tab[global_gain];
>> +            }
>
> The identical code is implemented here 3 times, 2 times with seperate
> LUTs and once by directly calling pow()
> The differenes look like bugs. like forgetting to check validity or
> performing the scaling needed for the float2int16 transform.
>
> if(cb[g][i] == NOISE_HCB && noise_flag-- > 0)
>    gain[index] += get_bits(gb, 9) - 256;
> else
>    gain[index] += get_vlc2(gb, ac->mainvlc.table, 7, 3) - 60;
> if(gain[index] > 255)
>    ...
> sf[g][i] = ac->pow2sf_tab[gain[index]];

I tried this and audibly it sounds fine on a few files I tested. I
want to compare the two tables and pow() * ac->sf_scale call before
submitting a patch though.

[...]

>> +static int coupling_channel_element(AACContext * ac, GetBitContext * gb, int id) {
>
>> +    float cc_scale[] = {
>> +        pow(2, 1/8.), pow(2, 1/4.), pow(2, 0.5), 2.
>> +    };
>
> static const
>
>
>> +    int num_gain = 0;
>> +    int c, g, sfb;
>> +    int sign;
>> +    float scale;
>> +    sce_struct * sce;
>> +    coupling_struct * coup;
>
>> +    if (!ac->che_cc[id]) {
>> +        return -1;
>> +    }
>
> somehow i think all these checks can be factored out.
>
>
>> +    sce = &ac->che_cc[id]->ch;
>> +    sce->mixing_gain = 1.0;
>> +
>> +    coup = &ac->che_cc[id]->coup;
>> +
>> +    coup->ind_sw = get_bits1(gb);
>> +    //if (coup->ind_sw)
>> +    //    av_log(ac->avccontext, AV_LOG_ERROR, "aac: independently switched coupling\n");
>> +    coup->num_coupled = get_bits(gb, 3);
>> +    for (c = 0; c <= coup->num_coupled; c++) {
>> +        num_gain++;
>> +        coup->is_cpe[c] = get_bits1(gb);
>> +        coup->tag_select[c] = get_bits(gb, 4);
>> +        if (coup->is_cpe[c]) {
>> +            coup->l[c] = get_bits1(gb);
>> +            coup->r[c] = get_bits1(gb);
>> +            if (coup->l[c] && coup->r[c])
>> +                num_gain++;
>> +        }
>> +    }
>> +    coup->domain = get_bits1(gb);
>> +    sign = get_bits(gb, 1);
>> +    scale = cc_scale[get_bits(gb, 2)];
>> +
>> +    if (individual_channel_stream(ac, gb, 0, 0, sce))
>> +        return -1;
>> +
>> +    for (c = 0; c < num_gain; c++) {
>> +        int cge = 1;
>> +        int gain = 0;
>> +        float gain_cache = 1.;
>> +        if (c != 0) {
>> +            cge = coup->ind_sw ? 1 : get_bits1(gb);
>> +            gain = cge ? get_vlc2(gb, ac->mainvlc.table, 7, 3) - 60: 0;
>
>> +            gain_cache = pow(scale, (float)gain);
>
> useless cast and cc_scale is useless too

I have two possibilities for this:

20080521-remove_cc_scale-scale_idx.diff
20080521-remove_cc_scale-scale.diff

The latter avoids a division within the loops at the cost of an
additional pow() call. I didn't know which would be preferred (though
I would lean toward the latter at a guess) so I'm submitting both for
review. :)

[...]

>> +static void tns_filter_tool(AACContext * ac, int decode, sce_struct * sce, float * coef) {
>
> bad name ...

The spec calls this function tns_decode_frame. Is that acceptable or
if not, do you have any hints? TNS == Temporal Noise Shaping.

[...]

Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20080521-remove_cc_scale-scale.diff
Type: text/x-diff
Size: 1706 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080521/d5a17a9c/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20080521-remove_cc_scale-scale_idx.diff
Type: text/x-diff
Size: 1844 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080521/d5a17a9c/attachment-0001.diff>