[FFmpeg-devel] [PATCH] HE-AAC v1 try 5

[Alex Converse]

On Thu, Mar 4, 2010 at 5:20 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Wed, Mar 03, 2010 at 10:07:12PM -0500, Alex Converse wrote:
[...]
>
>> ?avformat.h | ? ?2 +-
>> ?utils.c ? ?| ? ?5 +++++
>> ?2 files changed, 6 insertions(+), 1 deletion(-)
>> 4948a33990bbe6fca0e4bda563022b3e2e03d49f ?sbr_avformat.patch
>> From bfc85e165f3716abe08356a584dc8ac6e5593369 Mon Sep 17 00:00:00 2001
>> From: Alex Converse <alex.converse at gmail.com>
>> Date: Wed, 3 Mar 2010 05:17:59 -0500
>> Subject: [PATCH] Add a workaround for backwards compatible HE-AAC signaling.
[...]
>> --------------1
>
> ok if tested
>
>

ACK

Thanks for the feedback, most of it was very useful. However on a few
points I vehemently disagree.

Attached is an updated patch.

At this point we are significantly faster than faad. With your
approval I would like to commit this and move on to close the feature
gap with faad (work on ps).

> [...]
>> @@ -1651,7 +1640,21 @@ static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt)
>> ? ? ?case EXT_SBR_DATA_CRC:
>> ? ? ? ? ?crc_flag++;
>> ? ? ?case EXT_SBR_DATA:
>> - ? ? ? ?res = decode_sbr_extension(ac, gb, crc_flag, cnt);
>> + ? ? ? ?if (!che) {
>> + ? ? ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR, "SBR was found before the first channel element.\n");
>> + ? ? ? ? ? ?return res;
>> + ? ? ? ?} else if (!ac->m4ac.sbr) {
>> + ? ? ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR, "SBR signaled to be not-present but was found in the bitstream.\n");
>> + ? ? ? ? ? ?skip_bits_long(gb, 8 * cnt - 4);
>> + ? ? ? ? ? ?return res;
>> + ? ? ? ?} else if (ac->m4ac.sbr == -1 && ac->output_configured == OC_LOCKED) {
>> + ? ? ? ? ? ?av_log(ac->avccontext, 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 {
>> + ? ? ? ? ? ?ac->m4ac.sbr = 1;
>> + ? ? ? ?}
>> + ? ? ? ?res = ff_decode_sbr_extension(ac, &che->sbr, gb, crc_flag, cnt, elem_type);
>> ? ? ? ? ?break;
>> ? ? ?case EXT_DYNAMIC_RANGE:
>> ? ? ? ? ?res = decode_dynamic_range(&ac->che_drc, gb, cnt);
>
>> @@ -1833,7 +1836,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);
>> ?}
>>
>
> does this now always has 2x as much data or is half of this unneeded/wrong?
>
>

Fixed

> [...]
>> +
>> +static VLC vlc_sbr[10];
>> +static const int8_t vlc_sbr_lav[10] =
>> + ? ?{ 60, 60, 24, 24, 31, 31, 12, 12, 31, 12 };
>> +static DECLARE_ALIGNED_16(float, analysis_cos_pre)[64];
>> +static DECLARE_ALIGNED_16(float, analysis_sin_pre)[64];
>> +static DECLARE_ALIGNED_16(float, analysis_cossin_post)[32][2];
>
>> +static DECLARE_ALIGNED_16(float, zero64)[64];
>
> const
>

Fixed

>
> [...]
>> +static inline void remove_table_element_int16(int16_t *table, uint8_t *last_el,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int el)
>> +{
>> + ? ?memmove(table + el, table + el + 1, (*last_el - el)*sizeof(int16_t));
>> + ? ?(*last_el)--;
>> +}
>
> please use variable names that make sense, that is size or length not last_el
> and el should be index
>

el(ement) and last_el(ement) make plenty of sense to me.

>
>> +
>> +static inline int in_table_int16(int16_t *table, int last_el, int16_t needle)
>> +{
>> + ? ?int i;
>> + ? ?for (i = 0; i <= last_el; i++)
>> + ? ? ? ?if (table[i] == needle)
>> + ? ? ? ? ? ?return 1;
>> + ? ?return 0;
>> +}
>> +
>
>> +/// Limiter Frequency Band Table (14496-3 sp04 p198)
>> +static void sbr_make_f_tablelim(SpectralBandReplication *sbr)
>> +{
>> + ? ?int k;
>> + ? ?if (sbr->bs_limiter_bands > 0) {
>> + ? ? ? ?static const float lim_bands_per_octave[3] = {1.2, 2, 3};
>> + ? ? ? ?int16_t patch_borders[5];
>> +
>> + ? ? ? ?patch_borders[0] = sbr->k[4];
>> + ? ? ? ?for (k=1; k <= sbr->num_patches; k++)
>> + ? ? ? ? ? ?patch_borders[k] = patch_borders[k-1] + sbr->patch_num_subbands[k-1];
>> +
>
>> + ? ? ? ?memcpy( sbr->f_tablelim, ? ? ? ? ? ? ? ? ?sbr->f_tablelow,
>> + ? ? ? ? ? ? ? (sbr->n[0] ? ? ? ?+ 1) * sizeof(sbr->f_tablelow[0]));
>
> please dont place whitespace completely randomly
>

They used to line up before later changes. Fixed

>
>> + ? ? ? ?if (sbr->num_patches > 1)
>> + ? ? ? ? ? ?memcpy(&sbr->f_tablelim[sbr->n[0] + 1], &patch_borders[1],
>> + ? ? ? ? ? ? ? ? ? (sbr->num_patches - 1) * sizeof(patch_borders[0]));
>
> i think x+n is cleaner than &x[n]
>

Fixed

>
>> +
>> + ? ? ? ?qsort(sbr->f_tablelim, sbr->num_patches + sbr->n[0],
>> + ? ? ? ? ? ? ?sizeof(sbr->f_tablelim[0]),
>> + ? ? ? ? ? ? ?qsort_comparison_function_int16);
>> +
>
>> + ? ? ? ?k = 1;
>> + ? ? ? ?sbr->n_lim = sbr->n[0] + sbr->num_patches - 1;
>> + ? ? ? ?while (k <= sbr->n_lim) {
>> + ? ? ? ? ? ?// if ( nOctaves * limBands >= 0.49) ...
>> + ? ? ? ? ? ?if (log2(sbr->f_tablelim[k] / (float)sbr->f_tablelim[k-1]) *
>> + ? ? ? ? ? ? ? ?lim_bands_per_octave[sbr->bs_limiter_bands - 1] >= 0.49) {
>> + ? ? ? ? ? ? ? ?k++;
>> + ? ? ? ? ? ? ? ?continue;
>> + ? ? ? ? ? ?}
>
> X*L >= 0.49
> X >= 0.49/L
> X >= L' (which is 0.49 merged into the table)
> log2(a/b) >= L'
> a/b >= exp2(L')
> a >= b*L'' (which is exp2 merged into the table)
>

Fixed

>
>
>> + ? ? ? ? ? ?if (sbr->f_tablelim[k] == sbr->f_tablelim[k-1] ||
>> + ? ? ? ? ? ? ? ?!in_table_int16(patch_borders, sbr->num_patches, sbr->f_tablelim[k]))
>> + ? ? ? ? ? ? ? ?remove_table_element_int16(sbr->f_tablelim, &sbr->n_lim, k);
>> + ? ? ? ? ? ?else if (!in_table_int16(patch_borders, sbr->num_patches, sbr->f_tablelim[k-1]))
>> + ? ? ? ? ? ? ? ?remove_table_element_int16(sbr->f_tablelim, &sbr->n_lim, k-1);
>> + ? ? ? ? ? ?else
>> + ? ? ? ? ? ? ? ?k++;
>> + ? ? ? ?}
>
> this code seems somewhat obfuscated
> we have 2 tables A and B, they are combined into table C and sorted
> then starting from 0 to size all elements from C are removed that are
> too close to their previous element unless they where in B
>
> this can trivially be done in O(|C|) time your code though needs
> O(|C|*|C|) besides that the code seems messy which probably is the
> bigger issue.
> note, iam not asking you to make it faster if speed is irrelevant but
> it could be made less messy i think
>

I've simplified the logic slightly by removing the continue.
It looks O(|C||B|) and fairly clean to me. If you have an idea that's
less messy, please propose it.

>
>> + ? ?} else {
>> + ? ? ? ?sbr->f_tablelim[0] = sbr->f_tablelow[0];
>> + ? ? ? ?sbr->f_tablelim[1] = sbr->f_tablelow[sbr->n[0]];
>> + ? ? ? ?sbr->n_lim = 1;
>> + ? ?}
>> +}
>> +
>
>> +static unsigned int sbr_header(SpectralBandReplication *sbr, GetBitContext *gb)
>> +{
>> + ? ?unsigned int cnt = get_bits_count(gb);
>> + ? ?uint8_t bs_header_extra_1;
>> + ? ?uint8_t bs_header_extra_2;
>> + ? ?int old_bs_limiter_bands = sbr->bs_limiter_bands;
>> +
>> + ? ?sbr->start = 1;
>> +
>> + ? ?// Save last spectrum parameters variables to compare to new ones
>> + ? ?memcpy(&sbr->spectrum_params[0], &sbr->spectrum_params[1], sizeof(SpectrumParameters));
>
> spectrum_params[0] can be a local variable
>

Fixed

>
> [...]
>> +static int array_min_int16(int16_t *array, int nel)
>
> const int16_t *array
>

Fixed

>
>> +{
>> + ? ?int i, min = array[0];
>> + ? ?for (i = 1; i < nel; i++)
>
>> + ? ? ? ?if (array[i] < min)
>> + ? ? ? ? ? ?min = array[i];
>
> min= FFMIN(min, array[i]);

Fixed

>
> also i seriously doubt that the whole ac^k needs to be scaned for the
> minimum element, at the least you can stop once elements are more than 1 or
> 2 larger than the minimum, and that assumes rounding has a significant
> effect
>

If you wish to pursue this please provide a benchmark justifying the
extra complexity

>
> [...]
>> +
>> +/// Master Frequency Band Table (14496-3 sp04 p194)
>> +static int sbr_make_f_master(AACContext *ac, SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? SpectrumParameters *spectrum)
>> +{
>> + ? ?unsigned int temp;
>> + ? ?unsigned int start_min, stop_min;
>> + ? ?int k;
>> + ? ?const uint8_t *sbr_offset_ptr;
>> + ? ?int16_t stop_dk[13];
>> +
>> + ? ?if (sbr->sample_rate < 32000) {
>> + ? ? ? ?temp = 3000;
>> + ? ?} else if (sbr->sample_rate < 64000) {
>> + ? ? ? ?temp = 4000;
>> + ? ?} else
>> + ? ? ? ?temp = 5000;
>> +
>> + ? ?start_min = ((temp << 7) + (sbr->sample_rate >> 1)) / sbr->sample_rate;
>> + ? ?stop_min ?= ((temp << 8) + (sbr->sample_rate >> 1)) / sbr->sample_rate;
>> +
>> + ? ?switch (sbr->sample_rate) {
>> + ? ?case 16000:
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[0];
>> + ? ? ? ?break;
>> + ? ?case 22050:
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[1];
>> + ? ? ? ?break;
>> + ? ?case 24000:
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[2];
>> + ? ? ? ?break;
>> + ? ?case 32000:
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[3];
>> + ? ? ? ?break;
>> + ? ?case 44100: case 48000: case 64000:
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[4];
>> + ? ? ? ?break;
>> + ? ?case 88200: case 96000: case 128000: case 176400: case 192000:
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[5];
>> + ? ? ? ?break;
>> + ? ?default:
>> + ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "Unsupported sample rate for SBR: %d\n", sbr->sample_rate);
>> + ? ? ? ?return -1;
>> + ? ?}
>> +
>> + ? ?sbr->k[0] = start_min + sbr_offset_ptr[spectrum->bs_start_freq];
>> +
>> + ? ?if (spectrum->bs_stop_freq < 14) {
>> + ? ? ? ?sbr->k[2] = stop_min;
>
>> + ? ? ? ?make_bands(stop_dk, stop_min, 64, 13);
>> + ? ? ? ?qsort(stop_dk, 13, sizeof(stop_dk[0]), qsort_comparison_function_int16);
>
> just confirming, this array is not guranteed to be sorted already?
>

If it comes out of make_bands it isn't sorted.

If you inspect the history you will see adding these sorts fixed specific bugs.

>
>> + ? ? ? ?for (k = 0; k < spectrum->bs_stop_freq; k++)
>> + ? ? ? ? ? ?sbr->k[2] += stop_dk[k];
>> + ? ?} else if (spectrum->bs_stop_freq == 14) {
>> + ? ? ? ?sbr->k[2] = 2*sbr->k[0];
>> + ? ?} else if (spectrum->bs_stop_freq == 15) {
>> + ? ? ? ?sbr->k[2] = 3*sbr->k[0];
>> + ? ?} else {
>> + ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "Invalid bs_stop_freq: %d\n", spectrum->bs_stop_freq);
>> + ? ? ? ?return -1;
>> + ? ?}
>> + ? ?sbr->k[2] = FFMIN(64, sbr->k[2]);
>> +
>> + ? ?if (!spectrum->bs_freq_scale) {
>> + ? ? ? ?unsigned int dk;
>> + ? ? ? ?int k2diff;
>> +
>
>> + ? ? ? ?if (!spectrum->bs_alter_scale) {
>> + ? ? ? ? ? ?dk = 1;
>> + ? ? ? ? ? ?sbr->n_master = ?(sbr->k[2] - sbr->k[0]) & ~0x01;
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?dk = 2;
>> + ? ? ? ? ? ?sbr->n_master = ((sbr->k[2] - sbr->k[0] + 2) >> 2) << 1;
>> + ? ? ? ?}
>
> maybe silly idea but
> dk = 1 + spectrum->bs_alter_scale;
> sbr->n_master = ?(sbr->k[2] - sbr->k[0] + (2&dk))>>dk;
> sbr->n_master<<=1;
>
> does that with less code
>

Fixed

>
>> +
>> + ? ? ? ?for (k = 1; k <= sbr->n_master; k++)
>> + ? ? ? ? ? ?sbr->f_master[k] = dk;
>> +
>> + ? ? ? ?k2diff = sbr->k[2] - sbr->k[0] - sbr->n_master * dk;
>> + ? ? ? ?if (k2diff) {
>
>> + ? ? ? ? ? ?int incr;
>> + ? ? ? ? ? ?if (k2diff < 0) {
>> + ? ? ? ? ? ? ? ?incr = 1;
>> + ? ? ? ? ? ? ? ?k ? ?= 1;
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?incr = -1;
>> + ? ? ? ? ? ? ? ?k ? ?= sbr->n_master;
>> + ? ? ? ? ? ?}
>> +
>> + ? ? ? ? ? ?while (k2diff) {
>> + ? ? ? ? ? ? ? ?sbr->f_master[k] -= incr;
>> + ? ? ? ? ? ? ? ?k ? ? ? ? ? ? ? ?+= incr;
>> + ? ? ? ? ? ? ? ?k2diff ? ? ? ? ? += incr;
>> + ? ? ? ? ? ?}
>
> if(k2diff < 0){
> ? ?sbr->f_master[1]--;
> ? ?sbr->f_master[2]-= (k2diff < 1);
> }else{
> ? ?sbr->f_master[sbr->n_master]++;
> }
>

Fixed

>> + ? ? ? ?}
>> +
>> + ? ? ? ?sbr->f_master[0] = sbr->k[0];
>> + ? ? ? ?for (k = 1; k <= sbr->n_master; k++)
>> + ? ? ? ? ? ?sbr->f_master[k] += sbr->f_master[k - 1];
>> +
>> + ? ? ? ?// Requirements (14496-3 sp04 p205)
>> + ? ? ? ?if (sbr->n_master <= 0) {
>> + ? ? ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR, "Invalid n_master: %d\n", sbr->n_master);
>> + ? ? ? ? ? ?return -1;
>> + ? ? ? ?}
>> + ? ?} else {
>> + ? ? ? ?int half_bands = 7 - spectrum->bs_freq_scale; ? ? ?// bs_freq_scale ?= {1,2,3}
>> + ? ? ? ?int two_regions, num_bands_0;
>> + ? ? ? ?int vdk0_max, vdk1_min;
>> + ? ? ? ?int16_t vk0[49];
>> +
>
>> + ? ? ? ?if (sbr->k[2] / (float)sbr->k[0] > 2.2449f) {
>
> if (sbr->k[2] > 2.2449f*sbr->k[0]) {
>
> or maybe
> if (49*sbr->k[2] > 110*sbr->k[0]) {
> (but i did not check that this is correct for the input range)
>

The range is 1-64, which it seems to hold over
Fixed

>
>> + ? ? ? ? ? ?two_regions = 1;
>> + ? ? ? ? ? ?sbr->k[1] = 2 * sbr->k[0];
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?two_regions = 0;
>> + ? ? ? ? ? ?sbr->k[1] = sbr->k[2];
>> + ? ? ? ?}
>> +
>> + ? ? ? ?num_bands_0 = lrintf(half_bands * log2f(sbr->k[1] / (float)sbr->k[0])) * 2;
>> +
>> + ? ? ? ?if (num_bands_0 <= 0) { // Requirements (14496-3 sp04 p205)
>> + ? ? ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR, "Invalid num_bands_0: %d\n", num_bands_0);
>> + ? ? ? ? ? ?return -1;
>> + ? ? ? ?}
>> +
>> + ? ? ? ?vk0[0] = 0;
>> +
>
>> + ? ? ? ?make_bands(vk0+1, sbr->k[0], sbr->k[1], num_bands_0);
>> +
>> + ? ? ? ?qsort(vk0 + 1, num_bands_0, sizeof(vk0[1]), qsort_comparison_function_int16);
>
> sort needed?
>

See comment above on sorting

>
> [...]
>> + ? ?// temp == max number of QMF subbands
>> + ? ?if (sbr->sample_rate <= 32000) {
>> + ? ? ? ?temp = 48;
>> + ? ?} else if (sbr->sample_rate == 44100) {
>> + ? ? ? ?temp = 35;
>> + ? ?} else if (sbr->sample_rate >= 48000)
>> + ? ? ? ?temp = 32;
>> +
>> + ? ?if (sbr->k[2] - sbr->k[0] > temp) {
>> + ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "Invalid bitstream, too many QMF subbands: %d\n", sbr->k[2] - sbr->k[0]);
>> + ? ? ? ?return -1;
>> + ? ?}
>
>
>
>> +
>> + ? ?return 0;
>> +}
>> +
>> +/// 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];
>
> what is usb
Possibly upper subband?
> what is msb
Possibly middle subband?

They are not documents in the spec

>
>
>
>> + ? ?int goal_sb = ((1000 << 11) + (sbr->sample_rate >> 1)) / sbr->sample_rate;
>> +
>> + ? ?sbr->num_patches = 0;
>> +
>> + ? ?if (goal_sb < sbr->k[4] + sbr->m[1]) {
>> + ? ? ? ?for (k = 0; sbr->f_master[k] < goal_sb; k++) ;
>> + ? ?} else
>> + ? ? ? ?k = sbr->n_master;
>> +
>> + ? ?do {
>> + ? ? ? ?int odd = 0;
>> + ? ? ? ?for (i = k; i == k || sb > (sbr->k[0] - 1 + msb - odd); i--) {
>> + ? ? ? ? ? ?sb = sbr->f_master[i];
>> + ? ? ? ? ? ?odd = (sb - 2 + sbr->k[0]) & 1;
>
> the -2 is useless
>

Fixed

> and the condition can probably be written wthout odd
> maybe:
> sb > ((msb - 1)&~1) + sbr->k[0]
> works?
>

Is there an advantage to eliminating odd?

>
>> + ? ? ? ?}
>> +
>> + ? ? ? ?sbr->patch_num_subbands[sbr->num_patches] ?= FFMAX(sb - usb, 0);
>> + ? ? ? ?sbr->patch_start_subband[sbr->num_patches] = sbr->k[0] - odd - sbr->patch_num_subbands[sbr->num_patches];
>> +
>> + ? ? ? ?if (sbr->patch_num_subbands[sbr->num_patches] > 0) {
>> + ? ? ? ? ? ?usb = sb;
>> + ? ? ? ? ? ?msb = sb;
>> + ? ? ? ? ? ?sbr->num_patches++;
>> + ? ? ? ?} else
>> + ? ? ? ? ? ?msb = sbr->k[4];
>> +
>> + ? ? ? ?if (sbr->f_master[k] - sb < 3)
>> + ? ? ? ? ? ?k = sbr->n_master;
>> + ? ?} while (sb != sbr->k[4] + sbr->m[1]);
>> +
>
>> + ? ?if ((sbr->patch_num_subbands[sbr->num_patches-1] < 3) && (sbr->num_patches > 1))
>> + ? ? ? ?sbr->num_patches--;
>
> superflous ()
>

Fixed

>
> [...]
>> +static int sbr_grid(AACContext *ac, SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ?GetBitContext *gb, SBRData *ch_data)
>> +{
>> + ? ?int i;
>> +
>> + ? ?ch_data->bs_freq_res[0] = ch_data->bs_freq_res[ch_data->bs_num_env[1]];
>> + ? ?ch_data->bs_num_env[0] = ch_data->bs_num_env[1];
>> + ? ?ch_data->bs_amp_res = sbr->bs_amp_res_header;
>> +
>> + ? ?switch (ch_data->bs_frame_class = get_bits(gb, 2)) {
>> + ? ?case FIXFIX:
>> + ? ? ? ?ch_data->bs_num_env[1] = 1 << get_bits(gb, 2);
>> + ? ? ? ?if (ch_data->bs_num_env[1] == 1)
>> + ? ? ? ? ? ?ch_data->bs_amp_res = 0;
>> +
>> + ? ? ? ?ch_data->bs_freq_res[1] = get_bits1(gb);
>> + ? ? ? ?for (i = 1; i < ch_data->bs_num_env[1]; i++)
>> + ? ? ? ? ? ?ch_data->bs_freq_res[i + 1] = ch_data->bs_freq_res[1];
>> + ? ? ? ?break;
>> + ? ?case FIXVAR:
>> + ? ? ? ?ch_data->bs_var_bord[1] = get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_rel[1] ?= get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_env[1] ?= ch_data->bs_num_rel[1] + 1;
>> +
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_rel[1]; i++)
>> + ? ? ? ? ? ?ch_data->bs_rel_bord[1][i] = 2 * get_bits(gb, 2) + 2;
>> +
>> + ? ? ? ?ch_data->bs_pointer = get_bits(gb, ceil_log2[ch_data->bs_num_env[1]]);
>> +
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_env[1]; i++)
>> + ? ? ? ? ? ?ch_data->bs_freq_res[ch_data->bs_num_env[1] - i] = get_bits1(gb);
>> + ? ? ? ?break;
>> + ? ?case VARFIX:
>> + ? ? ? ?ch_data->bs_var_bord[0] = get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_rel[0] ?= get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_env[1] ?= ch_data->bs_num_rel[0] + 1;
>> +
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_rel[0]; i++)
>> + ? ? ? ? ? ?ch_data->bs_rel_bord[0][i] = 2 * get_bits(gb, 2) + 2;
>> +
>> + ? ? ? ?ch_data->bs_pointer = get_bits(gb, ceil_log2[ch_data->bs_num_env[1]]);
>> +
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_env[1]; i++)
>> + ? ? ? ? ? ?ch_data->bs_freq_res[i + 1] = get_bits1(gb);
>> + ? ? ? ?break;
>> + ? ?case VARVAR:
>> + ? ? ? ?ch_data->bs_var_bord[0] = get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_var_bord[1] = get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_rel[0] ?= get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_rel[1] ?= get_bits(gb, 2);
>> + ? ? ? ?ch_data->bs_num_env[1] ?= ch_data->bs_num_rel[0] + ch_data->bs_num_rel[1] + 1;
>> +
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_rel[0]; i++)
>> + ? ? ? ? ? ?ch_data->bs_rel_bord[0][i] = 2 * get_bits(gb, 2) + 2;
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_rel[1]; i++)
>> + ? ? ? ? ? ?ch_data->bs_rel_bord[1][i] = 2 * get_bits(gb, 2) + 2;
>> +
>> + ? ? ? ?ch_data->bs_pointer = get_bits(gb, ceil_log2[ch_data->bs_num_env[1]]);
>> +
>> + ? ? ? ?for (i = 0; i < ch_data->bs_num_env[1]; i++)
>> + ? ? ? ? ? ?ch_data->bs_freq_res[i + 1] = get_bits1(gb);
>> + ? ? ? ?break;
>> + ? ?}
>
> this can be done simpler and cleaner
>

This seems far simpler and cleaner than a maze of ifs. It also matches the spec.

>
>
>> +
>> + ? ?if (ch_data->bs_frame_class == FIXFIX && ch_data->bs_num_env[1] > 4) {
>> + ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "Invalid bitstream, too many SBR envelopes in FIXFIX type SBR frame: %d\n",
>> + ? ? ? ? ? ? ? ch_data->bs_num_env[1]);
>> + ? ? ? ?return -1;
>> + ? ?}
>> + ? ?if (ch_data->bs_frame_class == VARVAR && ch_data->bs_num_env[1] > 5) {
>> + ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "Invalid bitstream, too many SBR envelopes in VARVAR type SBR frame: %d\n",
>> + ? ? ? ? ? ? ? ch_data->bs_num_env[1]);
>> + ? ? ? ?return -1;
>> + ? ?}
>> +
>> + ? ?ch_data->bs_num_noise = (ch_data->bs_num_env[1] > 1) + 1;
>> +
>> + ? ?return 0;
>> +}
>> +
>> +static void sbr_grid_copy(SBRData *dst, const SBRData *src) {
>> + ? ?//These variables are saved from the previous frame rather than copied
>> + ? ?dst->bs_freq_res[0] = dst->bs_freq_res[dst->bs_num_env[1]];
>> + ? ?dst->bs_num_env[0] ?= dst->bs_num_env[1];
>> +
>
>> + ? ?//These variables are read from the bitstream and therefore copied
>> + ? ?memcpy(dst->bs_freq_res+1, src->bs_freq_res+1, sizeof(dst->bs_freq_res)-1);
>> + ? ?memcpy(dst->bs_num_env+1, ?src->bs_num_env+1, ?sizeof(dst->bs_num_env)-1);
>
> the len is only correct when its a (u)int8_t
>

And it is. What a "lucky" coincidence.

>
>> + ? ?memcpy(dst->bs_var_bord, ? src->bs_var_bord, ? sizeof(dst->bs_var_bord));
>> + ? ?memcpy(dst->bs_rel_bord, ? src->bs_rel_bord, ? sizeof(dst->bs_rel_bord));
>> + ? ?memcpy(dst->bs_num_rel, ? ?src->bs_num_rel, ? ?sizeof(dst->bs_rel_bord));
>> + ? ?dst->bs_amp_res ? ? = src->bs_amp_res;
>> + ? ?dst->bs_num_noise ? = src->bs_num_noise;
>> + ? ?dst->bs_pointer ? ? = src->bs_pointer;
>> + ? ?dst->bs_frame_class = src->bs_frame_class;
>> +}
>> +
>
>> +static void sbr_dtdf(SpectralBandReplication *sbr, GetBitContext *gb,
>> + ? ? ? ? ? ? ? ? ? ? SBRData *ch_data)
>
> i wonder what dtdf means
>

Comment added

>
>> +{
>> + ? ?int i;
>> +
>> + ? ?for (i = 0; i < ch_data->bs_num_env[1]; i++)
>> + ? ? ? ?ch_data->bs_df_env[i] ? = get_bits1(gb);
>> + ? ?for (i = 0; i < ch_data->bs_num_noise; i++)
>> + ? ? ? ?ch_data->bs_df_noise[i] = get_bits1(gb);
>> +}
>> +
>> +static void sbr_invf(SpectralBandReplication *sbr, GetBitContext *gb,
>> + ? ? ? ? ? ? ? ? ? ? SBRData *ch_data)
>> +{
>> + ? ?int i;
>> +
>> + ? ?memcpy(ch_data->bs_invf_mode[1], ch_data->bs_invf_mode[0], 5 * sizeof(uint8_t));
>> + ? ?for (i = 0; i < sbr->n_q; i++)
>> + ? ? ? ?ch_data->bs_invf_mode[0][i] = get_bits(gb, 2);
>> +}
>> +
>> +static void sbr_envelope(SpectralBandReplication *sbr, GetBitContext *gb,
>> + ? ? ? ? ? ? ? ? ? ? ? ? SBRData *ch_data, int ch)
>> +{
>> + ? ?int bits, max_depth;
>> + ? ?int i, j;
>> + ? ?VLC_TYPE (*t_huff)[2], (*f_huff)[2];
>> + ? ?int t_lav, f_lav;
>> +
>> + ? ?if (sbr->bs_coupling && ch) {
>> + ? ? ? ?max_depth = 2;
>> + ? ? ? ?if (ch_data->bs_amp_res) {
>> + ? ? ? ? ? ?bits = 5;
>> + ? ? ? ? ? ?t_huff = vlc_sbr[T_HUFFMAN_ENV_BAL_3_0DB].table;
>> + ? ? ? ? ? ?t_lav ?= vlc_sbr_lav[T_HUFFMAN_ENV_BAL_3_0DB];
>> + ? ? ? ? ? ?f_huff = vlc_sbr[F_HUFFMAN_ENV_BAL_3_0DB].table;
>> + ? ? ? ? ? ?f_lav ?= vlc_sbr_lav[F_HUFFMAN_ENV_BAL_3_0DB];
>
> can be vertically aligned

Fixed

>
>
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?bits = 6;
>> + ? ? ? ? ? ?t_huff = vlc_sbr[T_HUFFMAN_ENV_BAL_1_5DB].table;
>> + ? ? ? ? ? ?t_lav ?= vlc_sbr_lav[T_HUFFMAN_ENV_BAL_1_5DB];
>> + ? ? ? ? ? ?f_huff = vlc_sbr[F_HUFFMAN_ENV_BAL_1_5DB].table;
>> + ? ? ? ? ? ?f_lav ?= vlc_sbr_lav[F_HUFFMAN_ENV_BAL_1_5DB];
>> + ? ? ? ?}
>> + ? ?} else {
>> + ? ? ? ?max_depth = 3;
>> + ? ? ? ?if (ch_data->bs_amp_res) {
>> + ? ? ? ? ? ?bits = 6;
>> + ? ? ? ? ? ?t_huff = vlc_sbr[T_HUFFMAN_ENV_3_0DB].table;
>> + ? ? ? ? ? ?t_lav ?= vlc_sbr_lav[T_HUFFMAN_ENV_3_0DB];
>> + ? ? ? ? ? ?f_huff = vlc_sbr[F_HUFFMAN_ENV_3_0DB].table;
>> + ? ? ? ? ? ?f_lav ?= vlc_sbr_lav[F_HUFFMAN_ENV_3_0DB];
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?bits = 7;
>> + ? ? ? ? ? ?t_huff = vlc_sbr[T_HUFFMAN_ENV_1_5DB].table;
>> + ? ? ? ? ? ?t_lav ?= vlc_sbr_lav[T_HUFFMAN_ENV_1_5DB];
>> + ? ? ? ? ? ?f_huff = vlc_sbr[F_HUFFMAN_ENV_1_5DB].table;
>> + ? ? ? ? ? ?f_lav ?= vlc_sbr_lav[F_HUFFMAN_ENV_1_5DB];
>> + ? ? ? ?}
>> + ? ?}
>> +
>
>> + ? ?for (i = 0; i < ch_data->bs_num_env[1]; i++) {
>> + ? ? ? ?if (!ch_data->bs_df_env[i]) {
>> + ? ? ? ? ? ?ch_data->bs_data_env[i][0] = get_bits(gb, bits); // bs_env_start_value_balance
>> + ? ? ? ? ? ?for (j = 1; j < sbr->n[ch_data->bs_freq_res[i + 1]]; j++)
>> + ? ? ? ? ? ? ? ?ch_data->bs_data_env[i][j] = get_vlc2(gb, f_huff, 9, max_depth) - f_lav;
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?for (j = 0; j < sbr->n[ch_data->bs_freq_res[i + 1]]; j++)
>> + ? ? ? ? ? ? ? ?ch_data->bs_data_env[i][j] = get_vlc2(gb, t_huff, 9, max_depth) - t_lav;
>
> max_depth should be a constant, setting it to the maximum is faster
> than if it is variable
>

Fixed

>
> [...]
>> +static void sbr_sinusoidal_coding(SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GetBitContext *gb, SBRData *ch_data)
>> +{
>> + ? ?int i;
>> + ? ?for (i = 0; i < sbr->n[1]; i++)
>> + ? ? ? ?ch_data->bs_add_harmonic[i] = get_bits1(gb);
>> +}
>
> i guess this can be consdered a duplicate of all the other identical
> cases that read a single linear array of n bits
>

Factored out.
Maybe functionality should be added to get_bits.h?

>
> [...]
>> +static void sbr_single_channel_element(AACContext *ac,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? GetBitContext *gb)
>> +{
>> + ? ?if (get_bits1(gb)) // bs_data_extra
>> + ? ? ? ?skip_bits(gb, 4); // bs_reserved
>> +
>> + ? ?sbr_grid(ac, sbr, gb, &sbr->data[0]);
>> + ? ?sbr_dtdf(sbr, gb, &sbr->data[0]);
>> + ? ?sbr_invf(sbr, gb, &sbr->data[0]);
>> + ? ?sbr_envelope(sbr, gb, &sbr->data[0], 0);
>> + ? ?sbr_noise(sbr, gb, &sbr->data[0], 0);
>> +
>> + ? ?if ((sbr->data[0].bs_add_harmonic_flag = get_bits1(gb)))
>> + ? ? ? ?sbr_sinusoidal_coding(sbr, gb, &sbr->data[0]);
>> +}
>
> please name functions that primarely read bitstream data consistently
> like read_sbr_grid()
> so they can be distinguished from functions performing bitstream reader
> independant calculations like apply_mdct()
>

The fact that they take gb and the first or second parameter should be
a huge clue.
Never the less they have been renamed.

>
>> +
>> +static void sbr_channel_pair_element(AACContext *ac,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? GetBitContext *gb)
>> +{
>> + ? ?if (get_bits1(gb)) ? ?// bs_data_extra
>> + ? ? ? ?skip_bits(gb, 8); // bs_reserved
>> +
>
>> + ? ?if ((sbr->bs_coupling = get_bits1(gb))) {
>> + ? ? ? ?sbr_grid(ac, sbr, gb, &sbr->data[0]);
>> + ? ? ? ?sbr_grid_copy(&sbr->data[1], &sbr->data[0]);
>> + ? ? ? ?sbr_dtdf(sbr, gb, &sbr->data[0]);
>> + ? ? ? ?sbr_dtdf(sbr, gb, &sbr->data[1]);
>> + ? ? ? ?sbr_invf(sbr, gb, &sbr->data[0]);
>> + ? ? ? ?memcpy(sbr->data[1].bs_invf_mode[1], sbr->data[1].bs_invf_mode[0], sizeof(sbr->data[1].bs_invf_mode[0]));
>> + ? ? ? ?memcpy(sbr->data[1].bs_invf_mode[0], sbr->data[0].bs_invf_mode[0], sizeof(sbr->data[1].bs_invf_mode[0]));
>> + ? ? ? ?sbr_envelope(sbr, gb, &sbr->data[0], 0);
>> + ? ? ? ?sbr_noise(sbr, gb, &sbr->data[0], 0);
>> + ? ? ? ?sbr_envelope(sbr, gb, &sbr->data[1], 1);
>> + ? ? ? ?sbr_noise(sbr, gb, &sbr->data[1], 1);
>> + ? ?} else {
>> + ? ? ? ?sbr_grid(ac, sbr, gb, &sbr->data[0]);
>> + ? ? ? ?sbr_grid(ac, sbr, gb, &sbr->data[1]);
>> + ? ? ? ?sbr_dtdf(sbr, gb, &sbr->data[0]);
>> + ? ? ? ?sbr_dtdf(sbr, gb, &sbr->data[1]);
>> + ? ? ? ?sbr_invf(sbr, gb, &sbr->data[0]);
>> + ? ? ? ?sbr_invf(sbr, gb, &sbr->data[1]);
>> + ? ? ? ?sbr_envelope(sbr, gb, &sbr->data[0], 0);
>> + ? ? ? ?sbr_envelope(sbr, gb, &sbr->data[1], 1);
>> + ? ? ? ?sbr_noise(sbr, gb, &sbr->data[0], 0);
>> + ? ? ? ?sbr_noise(sbr, gb, &sbr->data[1], 1);
>> + ? ?}
>
> can be simplified by factorizing things out
>

Can be "simplified" at the expense of a more complicated control flow

>
> [...]
>> +/// SBR Envelope and Noise Floor Decoding (14496-3 sp04 p201)
>> +static void sbr_env_noise_floors(SpectralBandReplication *sbr, SBRData *ch_data,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int ch)
>> +{
>> + ? ?int delta = (ch == 1 && sbr->bs_coupling == 1) + 1;
>> + ? ?int i, k, L;
>> + ? ?const int temp = sbr->n[1] & 1;
>> + ? ?for (L = 0; L < ch_data->bs_num_env[1]; L++) {
>> + ? ? ? ?if (ch_data->bs_df_env[L]) {
>> + ? ? ? ? ? ?// bs_freq_res[0] == bs_freq_res[bs_num_env[1]] from prev frame
>> + ? ? ? ? ? ?if (ch_data->bs_freq_res[L + 1] == ch_data->bs_freq_res[L]) {
>> + ? ? ? ? ? ? ? ?for (k = 0; k < sbr->n[ch_data->bs_freq_res[L + 1]]; k++)
>> + ? ? ? ? ? ? ? ? ? ?ch_data->env_facs[L + 1][k] = ch_data->env_facs[L][k] + delta * ch_data->bs_data_env[L][k];
>> + ? ? ? ? ? ?} else if (ch_data->bs_freq_res[L + 1]) {
>> + ? ? ? ? ? ? ? ?for (k = 0; k < sbr->n[ch_data->bs_freq_res[L + 1]]; k++) {
>> + ? ? ? ? ? ? ? ? ? ?i = (k + temp) >> 1; // find i such that f_tablelow[i] <= f_tablehigh[k] < f_tablelow[i + 1]
>> + ? ? ? ? ? ? ? ? ? ?ch_data->env_facs[L + 1][k] = ch_data->env_facs[L][i] + delta * ch_data->bs_data_env[L][k];
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?for (k = 0; k < sbr->n[ch_data->bs_freq_res[L + 1]]; k++) {
>> + ? ? ? ? ? ? ? ? ? ?i = k ? 2*k - temp : 0; // find i such that f_tablehigh[i] == f_tablelow[k]
>> + ? ? ? ? ? ? ? ? ? ?ch_data->env_facs[L + 1][k] = ch_data->env_facs[L][i] + delta * ch_data->bs_data_env[L][k];
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?}
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?ch_data->env_facs[L + 1][0] = delta * ch_data->bs_data_env[L][0];
>> + ? ? ? ? ? ?for (k = 1; k < sbr->n[ch_data->bs_freq_res[L + 1]]; k++)
>> + ? ? ? ? ? ? ? ?ch_data->env_facs[L + 1][k] = ch_data->env_facs[L + 1][k - 1] + delta * ch_data->bs_data_env[L][k];
>> + ? ? ? ?}
>> + ? ?}
>
> please dont use l as variable name it looks like 1 in many fonts and
> makes the code harder to read.
> It is not reasonable to expect everyone who wishes to read the code to
> switch fonts or run a search and replace over the code, like i just did
> so i can review the code.
>

Find a font that doesn't suck. l is the canonical variable for
iterating over SBR envelopes.

> Back to the actual code, cant this be merged with reading the data from
> the bitstream? it seems the intermediate bs_data_env could at least be
> a local array
>

Merged with bitstream reading, bs_data_env removed

>
> [...]
>> +/// Dequantization and stereo decoding (14496-3 sp04 p203)
>> +static void sbr_dequant(SpectralBandReplication *sbr, int id_aac)
>> +{
>> + ? ?int k, L;
>> + ? ?int ch;
>> +
>> + ? ?if (id_aac == TYPE_CPE && sbr->bs_coupling) {
>
>> + ? ? ? ?float alpha = sbr->data[0].bs_amp_res ? 1.0f : 0.5f;
>> + ? ? ? ?float pan_offset = sbr->data[0].bs_amp_res ? 12.0f : 24.0f;
>
> vertical align
>

fixed

>
>> + ? ? ? ?for (L = 1; L <= sbr->data[0].bs_num_env[1]; L++) {
>> + ? ? ? ? ? ?for (k = 0; k < sbr->n[sbr->data[0].bs_freq_res[L]]; k++) {
>> + ? ? ? ? ? ? ? ?float temp1 = exp2f(sbr->data[0].env_facs[L][k] * alpha + 7.0f);
>> + ? ? ? ? ? ? ? ?float temp2 = exp2f((pan_offset - sbr->data[1].env_facs[L][k]) * alpha);
>
> isnt env_facs integer here? i so this can probably be done more
> efficiently with a LUT
>

env_facs can get quite large. The LAV of the delta coded version is
60. I'm not sure of the exact bounds.

>
> [...]
>> +/** High Frequency Generation (14496-3 sp04 p214+) and Inverse Filtering
>> + * (14496-3 sp04 p214)
>> + * Warning: This routine does not seem numerically stable.
>> + */
>> +static void sbr_hf_inverse_filter(float (*alpha0)[2], float (*alpha1)[2],
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?float X_low[32][40][2], int k0)
>
> const X_low ?

Fixed

>
>> +{
>> + ? ?int i, j, k, n;
>> + ? ?for (k = 0; k < k0; k++) {
>> + ? ? ? ?float phi[3][2][2], dk;
>> +
>> + ? ? ? ?for (i = 0; i < 3; i++) {
>> + ? ? ? ? ? ?for (j = 0; j < 2; j++) {
>> + ? ? ? ? ? ? ? ?unsigned int idxtmp1 = ENVELOPE_ADJUSTMENT_OFFSET - i;
>> + ? ? ? ? ? ? ? ?unsigned int idxtmp2 = ENVELOPE_ADJUSTMENT_OFFSET - (j + 1);
>> +
>> + ? ? ? ? ? ? ? ?phi[i][j][0] = 0.0f;
>> + ? ? ? ? ? ? ? ?phi[i][j][1] = 0.0f;
>
> these can be put inside the if() below
>

This code no longer exists in a form like that

>
>> +
>> + ? ? ? ? ? ? ? ?if (i <= j + 1)
>> + ? ? ? ? ? ? ? ?for (n = 0; n < 16 * 2 + 6; n++) {
>> + ? ? ? ? ? ? ? ? ? ?unsigned int idx1 = n + idxtmp1;
>> + ? ? ? ? ? ? ? ? ? ?unsigned int idx2 = n + idxtmp2;
>> + ? ? ? ? ? ? ? ? ? ?phi[i][j][0] += X_low[k][idx1][0] * X_low[k][idx2][0] +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?X_low[k][idx1][1] * X_low[k][idx2][1];
>> + ? ? ? ? ? ? ? ? ? ?if (i != j + 1) { // imaginary part
>> + ? ? ? ? ? ? ? ? ? ? ? ?phi[i][j][1] += X_low[k][idx1][1] * X_low[k][idx2][0] -
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?X_low[k][idx1][0] * X_low[k][idx2][1];
>> + ? ? ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?}
>> + ? ? ? ?}
>
> if i dont misread this then the implementation is not ideal
> you calcuate
> X[i0+n] * X[i1+n] for n=0..38
> and i0,i1 in 0,1 0,2 1,1 1,2 2,2
> here 1,1/2,2 and 0,1/1,2 calculate many multiplies twice
>
> also the if() and all the multidimendimensional array deref in the inner loop
> might have a bad effect on speed (depending on compiler)
>

I have rewritten this much more efficiently

>
> [...]
>> +/// Chirp Factors (14496-3 sp04 p214)
>> +static void sbr_chirp(SpectralBandReplication *sbr, SBRData *ch_data)
>> +{
>> + ? ?int i;
>> + ? ?float new_bw;
>> + ? ?float temp_bw;
>> +
>> + ? ?for (i = 0; i < sbr->n_q; i++) {
>> + ? ? ? ?switch (ch_data->bs_invf_mode[0][i]) {
>> + ? ? ? ?case 0:
>> + ? ? ? ? ? ?if (ch_data->bs_invf_mode[1][i] == 1) {
>> + ? ? ? ? ? ? ? ?new_bw = 0.6f;
>> + ? ? ? ? ? ?} else
>> + ? ? ? ? ? ? ? ?new_bw = 0.0f;
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case 1:
>> + ? ? ? ? ? ?if (!ch_data->bs_invf_mode[1][i]) {
>> + ? ? ? ? ? ? ? ?new_bw = 0.6f;
>> + ? ? ? ? ? ?} else
>> + ? ? ? ? ? ? ? ?new_bw = 0.75f;
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case 2:
>> + ? ? ? ? ? ?new_bw = 0.9f;
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?default:
>> + ? ? ? ? ? ?new_bw = 0.98f;
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?}
>
> if(ch_data->bs_invf_mode[0][i]+ch_data->bs_invf_mode[1][i] == 1){
> ? ?new_bw = 0.6f;
> }else{
> ? ?new_bw = table[ ch_data->bs_invf_mode[0][i] ];
> }
>
>

fixed

>
>
>> +
>> + ? ? ? ?if (new_bw < ch_data->bw_array[1][i]) {
>> + ? ? ? ? ? ?temp_bw = 0.75f ? ?* new_bw + 0.25f ? ?* ch_data->bw_array[1][i];
>> + ? ? ? ?} else
>> + ? ? ? ? ? ?temp_bw = 0.90625f * new_bw + 0.09375f * ch_data->bw_array[1][i];
>> + ? ? ? ?ch_data->bw_array[0][i] = temp_bw < 0.015625f ? 0.0f : temp_bw;
>> + ? ?}
>> +
>> + ? ?// update previous bw_array values
>> + ? ?memcpy(ch_data->bw_array[1], ch_data->bw_array[0], 5 * sizeof(float));
>
> why are there 2 arrays ? seems like you use just one
>

Fixed

>
>> +}
>> +
>> +static inline int find_freq_subband(uint16_t *table, int nel, int needle)
>> +{
>> + ? ?int i;
>> + ? ?for (i = 0; i < nel; i++) {
>> + ? ? ? ?if (needle >= table[i] && needle < table[i + 1])
>> + ? ? ? ? ? ?return i;
>> + ? ?}
>
> if i assume the table is sorted then you need just one of the 2 compares
> and you also seem to restart searching sequential elements from the start
>

Fixed

>
>> + ? ?return -1;
>> +}
>> +
>
>> +/// Generate the subband filtered lowband
>> +static int sbr_lf_gen(AACContext *ac, SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ?float X_low[32][40][2], float W[2][32][32][2]) {
>
> nothing being const looks suspicous
>

not being const is the easiest way to keep gcc from getting pissy
while looking for real warnings.
"Fixed"

>
>> + ? ?int k, L;
>> + ? ?const int t_HFGen = 8;
>> + ? ?const int l_f = 32;
>> + ? ?memset(X_low, 0, 32*sizeof(*X_low));
>> + ? ?for (k = 0; k < sbr->k[4]; k++) {
>> + ? ? ? ?for (L = t_HFGen; L < l_f + t_HFGen; L++) {
>> + ? ? ? ? ? ?X_low[k][L][0] = W[1][L - t_HFGen][k][0];
>> + ? ? ? ? ? ?X_low[k][L][1] = W[1][L - t_HFGen][k][1];
>> + ? ? ? ?}
>> + ? ?}
>> + ? ?for (k = 0; k < sbr->k[3]; k++) {
>> + ? ? ? ?for (L = 0; L < t_HFGen; L++) {
>> + ? ? ? ? ? ?X_low[k][L][0] = W[0][L + l_f - t_HFGen][k][0];
>> + ? ? ? ? ? ?X_low[k][L][1] = W[0][L + l_f - t_HFGen][k][1];
>> + ? ? ? ?}
>> + ? ?}
>
> the memset above seems to set some elements to 0 that are never read

It avoids per k calls to memset as well as extra logic at the bottom.
maybe rectangle filling functions can be used here?

>
> also cant this copying around be avoided and "W" be accessed directly
> by the code that uses X_low?
>

It breaks down to a trade off of what's more offensive, the extra
copying or all the extra logic needed to map X_low to W[0] W[1] or 0
appropriately.

>
>> + ? ?return 0;
>> +}
>> +
>> +/// High Frequency Generator (14496-3 sp04 p215)
>> +static int sbr_hf_gen(AACContext *ac, SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ?float X_high[64][40][2], float X_low[32][40][2], float (*alpha0)[2],
>> + ? ? ? ? ? ? ? ? ? ? ?float (*alpha1)[2], float bw_array[2][5], uint8_t *t_env,
>> + ? ? ? ? ? ? ? ? ? ? ?int bs_num_env)
>> +{
>> + ? ?int i, x, L;
>> + ? ?int k = sbr->k[4];
>> + ? ?for (i = 0; i < sbr->num_patches; i++) {
>> + ? ? ? ?for (x = 0; x < sbr->patch_num_subbands[i]; x++, k++) {
>> + ? ? ? ? ? ?const int g = find_freq_subband(sbr->f_tablenoise, sbr->n_q + 1, k);
>> + ? ? ? ? ? ?const int p = sbr->patch_start_subband[i] + x;
>> +
>> + ? ? ? ? ? ?if (g < 0) {
>> + ? ? ? ? ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? ? ? ? ? "ERROR : no subband found for frequency %d\n", k);
>> + ? ? ? ? ? ? ? ?return -1;
>> + ? ? ? ? ? ?}
>> +
>
>> + ? ? ? ? ? ?for (L = 2 * t_env[0]; L < 2 * t_env[bs_num_env]; L++) {
>> + ? ? ? ? ? ? ? ?const int idx = L + ENVELOPE_ADJUSTMENT_OFFSET;
>> + ? ? ? ? ? ? ? ?X_high[k][idx][0] =
>> + ? ? ? ? ? ? ? ? ? ?(X_low[p][idx - 2][0] * alpha1[p][0] -
>> + ? ? ? ? ? ? ? ? ? ? X_low[p][idx - 2][1] * alpha1[p][1]) * bw_array[0][g] * bw_array[0][g] +
>> + ? ? ? ? ? ? ? ? ? ?(X_low[p][idx - 1][0] * alpha0[p][0] -
>> + ? ? ? ? ? ? ? ? ? ? X_low[p][idx - 1][1] * alpha0[p][1]) * bw_array[0][g] +
>> + ? ? ? ? ? ? ? ? ? ? X_low[p][idx][0];
>> + ? ? ? ? ? ? ? ?X_high[k][idx][1] =
>> + ? ? ? ? ? ? ? ? ? ?(X_low[p][idx - 2][1] * alpha1[p][0] +
>> + ? ? ? ? ? ? ? ? ? ? X_low[p][idx - 2][0] * alpha1[p][1]) * bw_array[0][g] * bw_array[0][g] +
>> + ? ? ? ? ? ? ? ? ? ?(X_low[p][idx - 1][1] * alpha0[p][0] +
>> + ? ? ? ? ? ? ? ? ? ? X_low[p][idx - 1][0] * alpha0[p][1]) * bw_array[0][g] +
>> + ? ? ? ? ? ? ? ? ? ? X_low[p][idx][1];
>> + ? ? ? ? ? ?}
>
> bw_array can be multiplied into alpha cuting 6 multiplies away from this
> loop
>

Fixed

>
>> + ? ? ? ?}
>> + ? ?}
>> + ? ?if (k < sbr->m[1] + sbr->k[4])
>> + ? ? ? ?memset(X_high + k, 0, (sbr->m[1] + sbr->k[4] - k) * sizeof(*X_high));
>> +
>> + ? ?return 0;
>> +}
>> +
>
>> +/// Generate the subband filtered lowband
>> +static int sbr_x_gen(SpectralBandReplication *sbr,
>> + ? ? ? ? ? ? ? ? ? ? ?float X[2][32][64], float X_low[32][40][2], float Y[2][38][64][2], int ch) {
>> + ? ?int k, L;
>> + ? ?const int l_f = 32;
>> + ? ?const int l_Temp = FFMAX(2*sbr->data[ch].t_env_num_env_old - l_f, 0);
>> + ? ?memset(X, 0, 2*sizeof(*X));
>> + ? ?for (k = 0; k < sbr->k[3]; k++) {
>> + ? ? ? ?for (L = 0; L < l_Temp; L++) {
>> + ? ? ? ? ? ?X[0][L][k] = X_low[k][L + ENVELOPE_ADJUSTMENT_OFFSET][0];
>> + ? ? ? ? ? ?X[1][L][k] = X_low[k][L + ENVELOPE_ADJUSTMENT_OFFSET][1];
>> + ? ? ? ?}
>> + ? ?}
>> + ? ?for (; k < sbr->k[3] + sbr->m[0]; k++) {
>> + ? ? ? ?for (L = 0; L < l_Temp; L++) {
>> + ? ? ? ? ? ?X[0][L][k] = Y[0][L + l_f][k][0];
>> + ? ? ? ? ? ?X[1][L][k] = Y[0][L + l_f][k][1];
>> + ? ? ? ?}
>> + ? ?}
>> +
>> + ? ?for (k = 0; k < sbr->k[4]; k++) {
>> + ? ? ? ?for (L = l_Temp; L < l_f; L++) {
>> + ? ? ? ? ? ?X[0][L][k] = X_low[k][L + ENVELOPE_ADJUSTMENT_OFFSET][0];
>> + ? ? ? ? ? ?X[1][L][k] = X_low[k][L + ENVELOPE_ADJUSTMENT_OFFSET][1];
>> + ? ? ? ?}
>> + ? ?}
>> + ? ?for (; k < sbr->k[4] + sbr->m[1]; k++) {
>> + ? ? ? ?for (L = l_Temp; L < l_f; L++) {
>> + ? ? ? ? ? ?X[0][L][k] = Y[1][L][k][0];
>> + ? ? ? ? ? ?X[1][L][k] = Y[1][L][k][1];
>> + ? ? ? ?}
>> + ? ?}
>> + ? ?return 0;
>> +}
>
> more copying code ....

See above

> can that be avoided?
> and before you reply that the *dct needs it in that order in the next
> function, question is why is it not in that order to begin with?
>

Also in most o the rest of the code things are used in real/im pairs
so it makes sense to keep them next to eachother

>
> [...]
>> + ? ? ? ? ? ? ? ?for (i = ilb; i < iub; i++) {
>> + ? ? ? ? ? ? ? ? ? ?sum += X_high[m + sbr->k[4]][i][0] * X_high[m + sbr->k[4]][i][0] +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? X_high[m + sbr->k[4]][i][1] * X_high[m + sbr->k[4]][i][1];
>> + ? ? ? ? ? ? ? ?}
>
> dont we have a optimized funtion for taking the dot product of a
> real valued float vector alraedy?
>

With alignment and length constraints that can't be guaranteed.
Perhaps an unaligned, arbitrary length version would be a useful
addition to dsputil.

>
>> + ? ? ? ? ? ? ? ?e_curr[L][m] = sum * recip_env_size;
>> + ? ? ? ? ? ?}
>> + ? ? ? ?}
>> + ? ?} else {
>> + ? ? ? ?int k, p;
>> +
>> + ? ? ? ?for (L = 0; L < ch_data->bs_num_env[1]; L++) {
>> + ? ? ? ? ? ?const int env_size = 2 * (ch_data->t_env[L + 1] - ch_data->t_env[L]);
>> + ? ? ? ? ? ?int ilb = ch_data->t_env[L] ? ? * 2 + ENVELOPE_ADJUSTMENT_OFFSET;
>> + ? ? ? ? ? ?int iub = ch_data->t_env[L + 1] * 2 + ENVELOPE_ADJUSTMENT_OFFSET;
>> + ? ? ? ? ? ?const uint16_t *table = ch_data->bs_freq_res[L + 1] ? sbr->f_tablehigh : sbr->f_tablelow;
>> +
>> + ? ? ? ? ? ?for (p = 0; p < sbr->n[ch_data->bs_freq_res[L + 1]]; p++) {
>> + ? ? ? ? ? ? ? ?float sum = 0.0f;
>> + ? ? ? ? ? ? ? ?const int den = env_size * (table[p + 1] - table[p]);
>> +
>> + ? ? ? ? ? ? ? ?for (k = table[p]; k < table[p + 1]; k++) {
>
>> + ? ? ? ? ? ? ? ? ? ?for (i = ilb; i < iub; i++) {
>> + ? ? ? ? ? ? ? ? ? ? ? ?sum += X_high[k][i][0] * X_high[k][i][0] +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? X_high[k][i][1] * X_high[k][i][1];
>> + ? ? ? ? ? ? ? ? ? ?}
>
> duplicate

It's a simple dot product. There are also some duplicate i++s
scattered across the code as well.

>
>
> [...]
>> + ? ?static const int8_t phi[2][4] = {
>> + ? ? ? ?{ ?1, ?0, -1, ?0}, // real
>> + ? ? ? ?{ ?0, ?1, ?0, -1}, // imaginary
>> + ? ?};
>> + ? ?float (*g_temp)[48] = ch_data->g_temp, (*q_temp)[48] = ch_data->q_temp;
>> + ? ?int indexnoise = ch_data->f_indexnoise;
>> + ? ?int indexsine ?= ch_data->f_indexsine;
>> + ? ?memcpy(Y[0], Y[1], sizeof(Y[0]));
>> +
>> + ? ?if (sbr->reset) {
>> + ? ? ? ?for (i = 0; i < h_SL; i++) {
>> + ? ? ? ? ? ?memcpy(g_temp[i + 2*ch_data->t_env[0]], sbr->gain[0], m_max * sizeof(sbr->gain[0][0]));
>> + ? ? ? ? ? ?memcpy(q_temp[i + 2*ch_data->t_env[0]], sbr->q_m[0], ?m_max * sizeof(sbr->q_m[0][0]));
>> + ? ? ? ?}
>> + ? ?} else if (h_SL) {
>> + ? ? ? ?memcpy(g_temp[2*ch_data->t_env[0]], g_temp[2*ch_data->t_env_num_env_old], 4*sizeof(g_temp[0]));
>> + ? ? ? ?memcpy(q_temp[2*ch_data->t_env[0]], q_temp[2*ch_data->t_env_num_env_old], 4*sizeof(q_temp[0]));
>> + ? ?}
>> +
>> + ? ?for (L = 0; L < ch_data->bs_num_env[1]; L++) {
>> + ? ? ? ?for (i = 2 * ch_data->t_env[L]; i < 2 * ch_data->t_env[L + 1]; i++) {
>> + ? ? ? ? ? ?memcpy(g_temp[h_SL + i], sbr->gain[L], m_max * sizeof(sbr->gain[0][0]));
>> + ? ? ? ? ? ?memcpy(q_temp[h_SL + i], sbr->q_m[L], ?m_max * sizeof(sbr->q_m[0][0]));
>> + ? ? ? ?}
>> + ? ?}
>> +
>> + ? ?for (L = 0; L < ch_data->bs_num_env[1]; L++) {
>> + ? ? ? ?for (i = 2 * ch_data->t_env[L]; i < 2 * ch_data->t_env[L + 1]; i++) {
>> + ? ? ? ? ? ?int phi_sign = (1 - 2*(kx & 1));
>> +
>> + ? ? ? ? ? ?if (h_SL && L != l_a[0] && L != l_a[1]) {
>> + ? ? ? ? ? ? ? ?for (m = 0; m < m_max; m++) {
>> + ? ? ? ? ? ? ? ? ? ?const int idx1 = i + h_SL;
>> + ? ? ? ? ? ? ? ? ? ?float g_filt = 0.0f;
>> + ? ? ? ? ? ? ? ? ? ?for (j = 0; j <= h_SL; j++)
>> + ? ? ? ? ? ? ? ? ? ? ? ?g_filt += g_temp[idx1 - j][m] * h_smooth[j];
>> + ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][0] =
>> + ? ? ? ? ? ? ? ? ? ? ? ?X_high[m + kx][i + ENVELOPE_ADJUSTMENT_OFFSET][0] * g_filt;
>> + ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][1] =
>> + ? ? ? ? ? ? ? ? ? ? ? ?X_high[m + kx][i + ENVELOPE_ADJUSTMENT_OFFSET][1] * g_filt;
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?for (m = 0; m < m_max; m++) {
>> + ? ? ? ? ? ? ? ? ? ?const float g_filt = g_temp[i + h_SL][m];
>> + ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][0] =
>> + ? ? ? ? ? ? ? ? ? ? ? ?X_high[m + kx][i + ENVELOPE_ADJUSTMENT_OFFSET][0] * g_filt;
>> + ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][1] =
>> + ? ? ? ? ? ? ? ? ? ? ? ?X_high[m + kx][i + ENVELOPE_ADJUSTMENT_OFFSET][1] * g_filt;
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?}
>> +
>
>> + ? ? ? ? ? ?if (L != l_a[0] && L != l_a[1]) {
>> + ? ? ? ? ? ? ? ?for (m = 0; m < m_max; m++) {
>> + ? ? ? ? ? ? ? ? ? ?indexnoise = (indexnoise + 1) & 0x1ff;
>> + ? ? ? ? ? ? ? ? ? ?if (sbr->s_m[L][m]) {
>> + ? ? ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][0] +=
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?sbr->s_m[L][m] * phi[0][indexsine];
>> + ? ? ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][1] +=
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?sbr->s_m[L][m] * (phi[1][indexsine] * phi_sign);
>> + ? ? ? ? ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ? ? ? ? ?float q_filt;
>> + ? ? ? ? ? ? ? ? ? ? ? ?if (h_SL) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?const int idx1 = i + h_SL;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?q_filt = 0.0f;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?for (j = 0; j <= h_SL; j++)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?q_filt += q_temp[idx1 - j][m] * h_smooth[j];
>> + ? ? ? ? ? ? ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?q_filt = q_temp[i][m];
>> + ? ? ? ? ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][0] +=
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?q_filt * sbr_noise_table[indexnoise][0];
>> + ? ? ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][1] +=
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?q_filt * sbr_noise_table[indexnoise][1];
>> + ? ? ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ? ? ? ? ?phi_sign = -phi_sign;
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?indexnoise = (indexnoise + m_max) & 0x1ff;
>> + ? ? ? ? ? ? ? ?for (m = 0; m < m_max; m++) {
>> + ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][0] +=
>> + ? ? ? ? ? ? ? ? ? ? ? ?sbr->s_m[L][m] * phi[0][indexsine];
>> + ? ? ? ? ? ? ? ? ? ?Y[1][i][m + kx][1] +=
>> + ? ? ? ? ? ? ? ? ? ? ? ?sbr->s_m[L][m] * (phi[1][indexsine] * phi_sign);
>> + ? ? ? ? ? ? ? ? ? ?phi_sign = -phi_sign;
>> + ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?indexsine = (indexsine + 1) & 3;
>
> all the multiplies by phi* are 0,1,-1
> half of them are by 0
>
>

Additional loop unrolling can be done at a later date. This is
sufficient for the time being.

>
> [...]
>> +
>> +/**
>> + * Spectral Band Replication header - spectrum parameters that invoke a reset if they differ from the previous header.
>> + */
>> +typedef struct {
>> + ? ?uint8_t bs_start_freq;
>> + ? ?uint8_t bs_stop_freq;
>> + ? ?uint8_t bs_xover_band;
>> +
>> + ? ?/**
>> + ? ? * @defgroup bs_header_extra_1 ? ? Variables associated with bs_header_extra_1
>> + ? ? * @{
>> + ? ? */
>> + ? ?uint8_t bs_freq_scale;
>> + ? ?uint8_t bs_alter_scale;
>> + ? ?uint8_t bs_noise_bands;
>> + ? ?/** @} */
>> +} SpectrumParameters;
>> +
>> +#define SBR_SYNTHESIS_BUF_SIZE ((1280-128)*2)
>> +
>> +/**
>> + * Spectral Band Replication per channel data
>> + */
>> +typedef struct {
>> + ? ?/**
>> + ? ? * @defgroup bitstream ? ? Main bitstream data variables
>> + ? ? * @{
>> + ? ? */
>> + ? ?uint8_t ? ? ? ? ? ?bs_frame_class;
>> + ? ?uint8_t ? ? ? ? ? ?bs_add_harmonic_flag;
>> + ? ?uint8_t ? ? ? ? ? ?bs_num_env[2];
>> + ? ?uint8_t ? ? ? ? ? ?bs_freq_res[7];
>> + ? ?uint8_t ? ? ? ? ? ?bs_var_bord[2];
>> + ? ?uint8_t ? ? ? ? ? ?bs_num_rel[2];
>> + ? ?uint8_t ? ? ? ? ? ?bs_rel_bord[2][3];
>> + ? ?uint8_t ? ? ? ? ? ?bs_pointer;
>> + ? ?uint8_t ? ? ? ? ? ?bs_num_noise;
>> + ? ?uint8_t ? ? ? ? ? ?bs_df_env[5];
>> + ? ?uint8_t ? ? ? ? ? ?bs_df_noise[2];
>> + ? ?uint8_t ? ? ? ? ? ?bs_invf_mode[2][5];
>> + ? ?int32_t ? ? ? ? ? ?bs_data_env[7][32];
>> + ? ?int32_t ? ? ? ? ? ?bs_data_noise[2][5];
>> + ? ?uint8_t ? ? ? ? ? ?bs_add_harmonic[48];
>> + ? ?uint8_t ? ? ? ? ? ?bs_amp_res;
>> + ? ?/** @} */
>> +
>> + ? ?/**
>> + ? ? * @defgroup state ? ? ? ? State variables
>> + ? ? * @{
>> + ? ? */
>> + ? ?DECLARE_ALIGNED(16, float, synthesis_filterbank_samples)[SBR_SYNTHESIS_BUF_SIZE];
>> + ? ?DECLARE_ALIGNED(16, float, analysis_filterbank_samples) [1312];
>> + ? ?int ? ? ? ? ? ? ? ?synthesis_filterbank_samples_offset;
>> + ? ?int ? ? ? ? ? ? ? ?l_a[2];
>> + ? ?float ? ? ? ? ? ? ?bw_array[2][5];
>> + ? ?float ? ? ? ? ? ? ?W[2][32][32][2];
>> + ? ?float ? ? ? ? ? ? ?Y[2][38][64][2];
>> + ? ?float ? ? ? ? ? ? ?g_temp[42][48];
>> + ? ?float ? ? ? ? ? ? ?q_temp[42][48];
>> + ? ?uint8_t ? ? ? ? ? ?s_indexmapped[8][48];
>> + ? ?float ? ? ? ? ? ? ?env_facs[6][48];
>> + ? ?float ? ? ? ? ? ? ?noise_facs[3][5];
>> + ? ?uint8_t ? ? ? ? ? ?t_env[8];
>> + ? ?uint8_t ? ? ? ? ? ?t_env_num_env_old;
>> + ? ?uint8_t ? ? ? ? ? ?t_q[3];
>> + ? ?uint16_t ? ? ? ? ? f_indexnoise;
>> + ? ?uint8_t ? ? ? ? ? ?f_indexsine;
>> + ? ?/** @} */
>> +} SBRData;
>> +
>> +/**
>> + * Spectral Band Replication
>> + */
>> +typedef struct {
>> + ? ?int32_t ? ? ? ? ? ?sample_rate;
>> + ? ?uint8_t ? ? ? ? ? ?start;
>> + ? ?uint8_t ? ? ? ? ? ?reset;
>> + ? ?SpectrumParameters spectrum_params[2];
>> + ? ?uint8_t ? ? ? ? ? ?bs_amp_res_header;
>> + ? ?/**
>> + ? ? * @defgroup bs_header_extra_2 ? ? variables associated with bs_header_extra_2
>> + ? ? * @{
>> + ? ? */
>> + ? ?uint8_t ? ? ? ? ? ?bs_limiter_bands;
>> + ? ?uint8_t ? ? ? ? ? ?bs_limiter_gains;
>> + ? ?uint8_t ? ? ? ? ? ?bs_interpol_freq;
>> + ? ?uint8_t ? ? ? ? ? ?bs_smoothing_mode;
>> + ? ?/** @} */
>> + ? ?uint8_t ? ? ? ? ? ?bs_coupling;
>
>> + ? ?uint8_t ? ? ? ? ? ?k[5]; ///< k0, k1, k2, kx', and kx respectively
>
> this seems never used as an array, why is it one?
>

I don't know.

kx and kx' are together due to convention

> also why are so many variables fixed small sizes? this can cause a
> speed loss on some architectures, and even x86 with gcc seems to have a
> dislike for such things from what ive seen recently in h264 optims
>

My guess is it was done like this because the SBR context is so large
and per channel.

>
>> + ? ?uint8_t ? ? ? ? ? ?m[2]; ///< M' and M respectively
>> + ? ?uint8_t ? ? ? ? ? ?n_master;
>> + ? ?SBRData ? ? ? ? ? ?data[2];
>> + ? ?uint8_t ? ? ? ? ? ?n[2]; ///< n_low and n_high respectively
>> + ? ?uint8_t ? ? ? ? ? ?n_q;
>> + ? ?uint8_t ? ? ? ? ? ?n_lim;
>> + ? ?uint16_t ? ? ? ? ? f_master[49];
>> + ? ?uint16_t ? ? ? ? ? f_tablelow[25];
>> + ? ?uint16_t ? ? ? ? ? f_tablehigh[49];
>> + ? ?uint16_t ? ? ? ? ? f_tablenoise[6];
>> + ? ?uint16_t ? ? ? ? ? f_tablelim[29];
> [...]
>> + ? ?float ? ? ? ? ? ? ?X_low[32][40][2];
>> + ? ?float ? ? ? ? ? ? ?X_high[64][40][2];
>> + ? ?DECLARE_ALIGNED(16, float, X)[2][32][64];
>> + ? ?float ? ? ? ? ? ? ?alpha0[64][2];
>> + ? ?float ? ? ? ? ? ? ?alpha1[64][2];
>> + ? ?float ? ? ? ? ? ? ?e_origmapped[7][48];
>> + ? ?float ? ? ? ? ? ? ?q_mapped[7][48];
>> + ? ?uint8_t ? ? ? ? ? ?s_mapped[7][48];
>> + ? ?float ? ? ? ? ? ? ?e_curr[7][48];
>> + ? ?float ? ? ? ? ? ? ?q_m[7][48];
>> + ? ?float ? ? ? ? ? ? ?s_m[7][48];
>
> It would tremendously help code readability and thus also me reviewing
> if these variables where things that made sense and did not require one
> to look _each_ up in a spec that isnt freely available.
>

I find it much easier when writing the code and fixing bugs if the
variables do match the spec rather than having to look across some
equivalence table.

> A little like FOSS encrypted with a key one has to pay for ...

If you are dissatisfied with the price of the specification, please
contact your national standards body and your SC29 delegation.

A free draft is available at http://www.mp3-tech.org/programmer/docs/w4611.pdf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbr.diff
Type: text/x-patch
Size: 132287 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100305/bfec6b1e/attachment.bin>