[FFmpeg-devel] [PATCH] AAC decoder round 8

Robert Swain robert.swain
Mon Aug 18 14:22:01 CEST 2008


2008/8/15 Robert Swain <robert.swain at gmail.com>:
> 2008/8/15 Michael Niedermayer <michaelni at gmx.at>:
>> On Fri, Aug 15, 2008 at 09:04:24AM +0100, Robert Swain wrote:
>>> 2008/8/15 Michael Niedermayer <michaelni at gmx.at>:
>>> > On Fri, Aug 15, 2008 at 01:32:08AM +0100, Robert Swain wrote:
>>> >> $subj
>>> >>
>>> >> There's not much left to commit now! :D
>>> >
>>> > ok
>>>
>>> All committed. Just to make it easier for me and/or you to keep track
>>> of, here's another patch attached with the remaining hunks.
>>>
>>> Regards,
>>> Rob
>>
>>> Index: libavcodec/aac.c
>>> ===================================================================
>>> --- libavcodec/aac.c  (revision 14774)
>>> +++ libavcodec/aac.c  (working copy)
>> [...]
>>> @@ -605,6 +616,44 @@
>>>  }
>>>
>>>  /**
>>> + * Decode Temporal Noise Shaping data; reference: table 4.48.
>>> + *
>>> + * @return  Returns error status. 0 - OK, !0 - error
>>> + */
>>> +static int decode_tns(AACContext * ac, TemporalNoiseShaping * tns,
>>> +        GetBitContext * gb, const IndividualChannelStream * ics) {
>>
>>> +    int w, filt, i, coef_len, coef_res = 0, coef_compress;
>>
>> useless init ?
>
> Subtle, but yes.
>
>>> +    const int is8 = ics->window_sequence[0] == EIGHT_SHORT_SEQUENCE;
>>> +    const int tns_max_order = is8 ? 7 : ac->m4ac.object_type == AOT_AAC_MAIN ? 20 : 12;
>>> +    for (w = 0; w < ics->num_windows; w++) {
>>> +        tns->n_filt[w] = get_bits(gb, 2 - is8);
>>> +
>>> +        if (tns->n_filt[w])
>>> +            coef_res = get_bits1(gb) + 3;
>>> +
>>> +        for (filt = 0; filt < tns->n_filt[w]; filt++) {
>>> +            tns->length[w][filt] = get_bits(gb, 6 - 2*is8);
>>> +
>>
>>> +            if ((tns->order[w][filt] = get_bits(gb, 5 - 2*is8)) <= tns_max_order) {
>>> +                tns->direction[w][filt] = get_bits1(gb);
>>> +                coef_compress = get_bits1(gb);
>>> +                coef_len = coef_res - coef_compress;
>>> +                tns->tmp2_map[w][filt] = tns_tmp2_map[2*coef_compress + coef_res - 3];
>>
>> the 3 can be moved to "coef_len = coef_res - coef_compress + 3"
>
> But, unless I'm missing something, that will change the behaviour of
> the code as more bits will be read in the loop you quoted just below.
>
>>> +
>>> +                for (i = 0; i < tns->order[w][filt]; i++)
>>> +                    tns->coef[w][filt][i] = get_bits(gb, coef_len);
>>
>> tns->coef is only used to index into tmp2_map thus
>> tns->coef could already contain the values from tmp2_map
>> this also would make the tmp2_map field unneeded in the struct
>
> OK, I'll look into this.
>
>>> +            } else {
>>> +                av_log(ac->avccontext, AV_LOG_ERROR, "TNS filter order %d is greater than maximum %d.",
>>> +                       tns->order[w][filt], tns_max_order);
>>> +                tns->order[w][filt] = 0;
>>> +                return -1;
>>> +            }
>>
>> if(... > tns_max_order){
>>    ...
>>    return -1
>> }
>> ...
>>
>> seems cleaner to me
>
> Done.
>
>>> +        }
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>>   * Decode Mid/Side data; reference: table 4.54.
>>>   *
>>>   * @param   ms_present  Indicates mid/side stereo presence. [0] mask is all 0s;
>>
>>> @@ -1067,6 +1116,71 @@
>>>  }
>>>
>>>  /**
>>> + * Decode Temporal Noise Shaping filter coefficients and apply all-pole filters; reference: 4.6.9.3.
>>> + *
>>> + * @param   decode  1 if tool is used normally, 0 if tool is used in LTP.
>>> + * @param   coef    spectral coefficients
>>> + */
>>> +static void apply_tns(float coef[1024], TemporalNoiseShaping * tns, IndividualChannelStream * ics, int decode) {
>>> +    const int mmm = FFMIN(ics->tns_max_bands,  ics->max_sfb);
>>> +    int w, filt, m, i, ib;
>>> +    int bottom, top, order, start, end, size, inc;
>>> +    float tmp;
>>> +    float lpc[TNS_MAX_ORDER + 1], b[2 * TNS_MAX_ORDER];
>>> +
>>> +    for (w = 0; w < ics->num_windows; w++) {
>>> +        bottom = ics->num_swb;
>>> +        for (filt = 0; filt < tns->n_filt[w]; filt++) {
>>> +            top = bottom;
>>> +            bottom = FFMAX(                  0, top - tns->length[w][filt]);
>>
>>> +            order  = FFMIN(tns->order[w][filt], TNS_MAX_ORDER);
>>
>> useless?
>
> Indeed. Removed. Should the 'order' variable be removed as well or is
> accessing it faster than accessing order[][]?
>
>>> +            if (order == 0)
>>> +                continue;
>>> +
>>
>>> +            // tns_decode_coef
>>> +            lpc[0] = 1;
>>> +            for (m = 1; m <= order; m++) {
>>> +                lpc[m] = tns->tmp2_map[w][filt][tns->coef[w][filt][m - 1]];
>>> +                for (i = 1; i < m; i++)
>>> +                    b[i] = lpc[i] + lpc[m] * lpc[m-i];
>>> +                for (i = 1; i < m; i++)
>>> +                    lpc[i] = b[i];
>>> +            }
>>
>> looks a little like eval_coefs from ra144.c
>> but later is fixedpoint, so this is more a random comment than anything
>>
>>
>>> +
>>> +            start = ics->swb_offset[FFMIN(bottom, mmm)];
>>> +            end   = ics->swb_offset[FFMIN(   top, mmm)];
>>> +            if ((size = end - start) <= 0)
>>> +                continue;
>>> +            if (tns->direction[w][filt]) {
>>> +                inc = -1; start = end - 1;
>>> +            } else {
>>> +                inc = 1;
>>> +            }
>>> +            start += w * 128;
>>> +
>>> +            // ar filter
>>> +            memset(b, 0, sizeof(b));
>>> +            ib = 0;
>>
>>> +            for (m = 0; m < size; m++) {
>>> +                tmp = coef[start];
>>> +                if (decode) {
>>> +                    for (i = 0; i < order; i++)
>>> +                        tmp -= b[ib + i] * lpc[i + 1];
>>> +                } else { // encode
>>> +                    for (i = 0; i < order; i++)
>>> +                        tmp += b[i]      * lpc[i + 1];
>>> +                }
>>> +                if (--ib < 0)
>>> +                    ib = order - 1;
>>> +                b[ib] = b[ib + order] = tmp;
>>> +                coef[start] = tmp;
>>> +                start += inc;
>>> +            }
>>
>> decode is always 1
>
> This is left from LTP. I'll remove it when submitting next.
>
>> b is not truly needed, coef[] can be used i its place
>> also this is likely relevant to overal codec speed so it
>> should be written more with speed than compactness in mind
>
> Does that mean you want me to rewrite it?

Without the encode case it would look like this, I think:

memset(b, 0, sizeof(b));
ib = 0;
for (m = 0; m < size; m++) {
    tmp = coef[start];
    for (i = 0; i < order; i++)
        tmp -= b[i] * lpc[i + 1];
    if (--ib < 0)
        ib = order - 1;
    coef[start] = b[ib] = tmp;
    start += inc;
}

Then I don't think tmp is needed or b as none of the values of coeff
but the one currently being filtered are changed and that one is only
subtracted from, it's not involved in any calculations, so I think the
following is correct:

for (m = 0; m < size; m++) {
    for (i = 0; i < FFMIN(m, order); i++)
        coef[start] -= coef[start - 1 - i] * lpc[i + 1];
    start += inc;
}

The loop limit, FFMIN(m, order), is to mimic the memset(b, 0,
sizeof(b)) behaviour. It has the bonus of being faster than the old
implementation too as it avoids some 0 * lpc[i+1]. :)

Rob




More information about the ffmpeg-devel mailing list