[FFmpeg-devel] [PATCH] AAC Decoder round 3

[Robert Swain]

2008/7/6 Michael Niedermayer <michaelni at gmx.at>:
> On Tue, Jul 01, 2008 at 12:37:11PM +0100, Robert Swain wrote:
> [...]
>> +/**
>> + * Free a channel element.
>> + */
>> +static void che_freep(ChannelElement **s) {
>> +    if(!*s)
>> +        return;
>> +    av_freep(s);
>> +}
>
> am i too tired or is the if-return useless and this a senseless wraper
> function?

I'll remove it for the next submission but it's needed for SSR/LTP in
SoC so I won't remove it there.

> [...]
>> +/**
>> + * Decode section_data payload; reference: table 4.46.
>> + */
>> +static int decode_section_data(AACContext * ac, GetBitContext * gb, IndividualChannelStream * ics, int cb[][64], int cb_run_end[][64]) {
>> +    int g;
>> +    for (g = 0; g < ics->num_window_groups; g++) {
>> +        int bits = (ics->window_sequence == EIGHT_SHORT_SEQUENCE) ? 3 : 5;
>> +        int k = 0;
>> +        while (k < ics->max_sfb) {
>> +            int sect_len = k;
>> +            int sect_len_incr;
>> +            int sect_cb = get_bits(gb, 4);
>> +            if (sect_cb == 12) {
>> +                av_log(ac->avccontext, AV_LOG_ERROR, "invalid codebook\n");
>> +                return -1;
>> +            }
>
>> +            while ((sect_len_incr = get_bits(gb, bits)) == (1 << bits)-1)
>> +                sect_len += sect_len_incr;
>> +            sect_len += sect_len_incr;
>> +            if (sect_len > ics->max_sfb) {
>
> change the check to unsigned or check for < 0 please, it doesnt seeem to
> do any harm but <0 is not correct

Made sect_len and max_sfb uint8_t as they should be.

> [...]
>> +/**
>> + * Decode scale_factor_data; reference: table 4.47.
>> + */
>> +static int decode_scale_factor_data(AACContext * ac, GetBitContext * gb, float mix_gain, unsigned int global_gain, IndividualChannelStream * ics, const int cb[][64], const int cb_run_end[][64], float sf[][64]) {
>> +    const int sf_offset = ac->sf_offset + (ics->window_sequence == EIGHT_SHORT_SEQUENCE ? 12 : 0);
>> +    int g, i;
>> +    int offset[3] = { global_gain, global_gain - 90, 100 };
>> +    int noise_flag = 1;
>> +    static const char *sf_str[3] = { "Global gain", "Noise gain", "Intensity stereo position" };
>> +    ics->intensity_present = 0;
>> +    for (g = 0; g < ics->num_window_groups; g++) {
>> +        for (i = 0; i < ics->max_sfb;) {
>> +            if (cb[g][i] == ZERO_HCB) {
>> +                int run_end = cb_run_end[g][i];
>> +                for(; i < run_end; i++)
>> +                    sf[g][i] = 0.;
>> +                continue;
>
> useless continue and this could use a memset(0)

I didn't use memset because the runs are usually small (<10) and I
thought using memset in such cases may be slower than just assigning
the values. I can change it to memset if you wish though.

> [...]
>> +static void coupling_tool(AACContext * ac, int independent, int domain) {
>
> This as well as other functions could benefit from some documentation
> what does it do, what is "independent" what "domain" ?

independent == 0 : dependent channel coupling
independent == 1 : independent channel coupling

The dependence of the channel coupling has implications on the data
that is shared between the coupled channels and how the data is
processed.

domain == 0 : apply coupling before TNS decoding
domain == 1 : apply coupling after TNS decoding

This variable is called cc_domain in the spec.

If you're not happy with the names, please make suggestions based on
the descriptions. I have added the documentation of these particular
ones and I'm looking at other non-obvious function names and adding
doxygen comments.

>> +    int i;
>> +    for (i = 0; i < MAX_TAGID; i++) {
>> +        ChannelElement * cc = ac->che[ID_CCE][i];
>> +        if (cc) {
>> +            if (cc->coup.ind_sw && independent) {
>> +                transform_coupling_tool(ac, cc, coupling_independent_trans);
>> +            } else if (!cc->coup.ind_sw && !independent && (cc->coup.domain == domain)) {
>> +                transform_coupling_tool(ac, cc, coupling_dependent_trans);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>
>> +static void transform_sce_tool(AACContext * ac, void (*sce_trans)(AACContext * ac, SingleChannelElement * sce)) {
>
> iam still not happy with _tool() functions. Functions should be named
> so that the names describes what the function does.

They're called tools in the spec. Looking up blah_tool shouldn't be
too difficult, but I agree it would be better if one didn't have to
look up the meaning of a function name. I'll see what I can do about
this tomorrow.

> [...]
>> +static void spec_to_sample(AACContext * ac) {
>
> what is "spec to sample" ? the name does not say anythig to me

A function that converts spectral data to time domain sample data?
I'll annotate it as I will do for other functions.

> [...]
>
>> +    i = 1024 * avccontext->channels * sizeof(uint16_t);
>> +    if(*data_size < i)
>> +        return -1;
>
> i wonder if this shouldnt be checked earlier (closer to where the number of
> channels is set)

data_size is only known in aac_decode_frame() and avctx->channels is
known at the end of output_configure() which is called at the end of
program_config_element() and program_config_element_default().

The latter is called during aac_decode_init() and imposes the default
channel mapping. A stream doesn't have to have a program config
element to reconfigure the channel mappings from the default (which is
the whole point of having default mappings I guess).

We have to be certain of the number of channels before we can do this check.

The spec says that a program config element must come before all other
syntactic elements in the while() loop in aac_decode_frame(). So, I
can add a flag var which can be toggled after the first syntactic
element has been decoded and check data_size then or I can add the
check just after the while() loop or maybe you have another
suggestion?

Regards,
Rob