[FFmpeg-soc] AACSBR review: sbr_time_freq_grid

[Alex Converse]

This function seems pretty fishy to me...

> // Time/frequency Grid (14496-3 sp04 p200)
> static int sbr_time_freq_grid(AACContext *ac, SpectralBandReplication *sbr,
>                               SBRData *ch_data, int ch)
> {
>     unsigned int abs_bord_lead  =  ch_data->bs_frame_class >= 2 ? ch_data->bs_var_bord[0] : 0;
>     // frameLengthFlag ? 15 : 16; 960 sample length frames unsupported; this value is numTimeSlots
>     unsigned int abs_bord_trail = (ch_data->bs_frame_class & 1 ? ch_data->bs_var_bord[1] : 0) + 16;
>     unsigned int n_rel_lead, n_rel_trail;
>     int i;
>
>     if (ch_data->bs_frame_class == FIXFIX) {
>         n_rel_lead = ch_data->bs_num_env[1] - 1;
>     } else if (ch_data->bs_frame_class == FIXVAR) {
>         n_rel_lead = 0;
>     } else if (ch_data->bs_frame_class < 4) { // VARFIX or VARVAR
>         n_rel_lead = ch_data->bs_num_rel[0];
>     } else {
>         av_log(ac->avccontext, AV_LOG_ERROR, "Invalid bs_frame_class for SBR: %d\n", ch_data->bs_frame_class);
>         return -1;
>     }
>
>     n_rel_trail = ch_data->bs_frame_class & 1 ? ch_data->bs_num_rel[1] : 0;

This is never read

>
>     sbr->t_env[ch][0]                      = abs_bord_lead;
>     sbr->t_env[ch][ch_data->bs_num_env[1]] = abs_bord_trail;
>
>     if (ch_data->bs_frame_class == FIXFIX) {
>         unsigned int temp = (unsigned int)lrintf(abs_bord_trail / (float)ch_data->bs_num_env[1]);

I believe the spec does specify rounding direction here.

>         for (i = 0; i < n_rel_lead; i++)
>             sbr->t_env[ch][i + 1] = sbr->t_env[ch][i] + temp;
>     } else if (ch_data->bs_frame_class > 1) { // VARFIX or VARVAR
>         for (i = 0; i < n_rel_lead; i++)
>             sbr->t_env[ch][i + 1] = sbr->t_env[ch][i] + ch_data->bs_rel_bord[0][i];
>     } else { // FIXVAR
>         for (i = 0; i < n_rel_lead; i++)
>             sbr->t_env[ch][i + 1] = abs_bord_lead;
>     }
>
>     if (ch_data->bs_frame_class & 1) { // FIXVAR or VARVAR
>         for (i = ch_data->bs_num_env[1] - 1; i > n_rel_lead; i--)
>             sbr->t_env[ch][i] = sbr->t_env[ch][i + 1] - ch_data->bs_rel_bord[1][ch_data->bs_num_env[1] - 1 - i];
>     } else { // FIXFIX or VARFIX
>         for (i = n_rel_lead; i < ch_data->bs_num_env[1]; i++)
>             sbr->t_env[ch][i + 1] = abs_bord_trail;
>     }

Are these fencepost ok?
My spec seems to partition the space i=0, 1 <= i <= nRelLead, nRelLead
< i < bs_num_env, i = bs_num_env

>
>     sbr->t_q[ch][0] = sbr->t_env[ch][0];
>     if (ch_data->bs_num_noise > 1) { // typo in spec bases this on bs_num_env...
>         unsigned int idx;
>         if (ch_data->bs_frame_class == FIXFIX) {
>             idx = ch_data->bs_num_env[1] >> 1;
>         } else if (ch_data->bs_frame_class & 1) { // FIXVAR or VARVAR
>             idx = ch_data->bs_num_env[1] - FFMAX(ch_data->bs_pointer - 1, 1);
>         } else { // VARFIX
>             if (!ch_data->bs_pointer)
>                 idx = 1;
>             else if (ch_data->bs_pointer == 1)
>                 idx = ch_data->bs_num_env[1] - 1;
>             else // bs_pointer > 1
>                 idx = ch_data->bs_pointer - 1;
>         }
>         sbr->t_q[ch][1] = sbr->t_env[ch][idx];
>         sbr->t_q[ch][2] = sbr->t_env[ch][ch_data->bs_num_env[1]];
>     } else
>         sbr->t_q[ch][1] = sbr->t_env[ch][ch_data->bs_num_env[1]];
> }