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

[Alex Converse]

On Mon, Mar 8, 2010 at 5:01 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Mar 07, 2010 at 11:26:16PM -0500, Alex Converse wrote:
>> On Sat, Mar 6, 2010 at 4:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Fri, Mar 05, 2010 at 07:00:14PM -0500, Alex Converse wrote:
>> >> 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:
[...]
>> >> >> +
>> >> >> +/// 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?
>> >
>> > "possibly"? you dont know what the code you submit means?
>> >
>>
>> I've made no attempts to conceal the fact that I am not the author of
>> a substantial portion of this code.
>
> yeah but the author of that part looks like someone from the standards
> comitee, that is under gpl? bsd? public domain? "non free"?
>

Rob wrote C code based on a flowchart in the standards text. The
identifier names are taken from the text. I'm curious as to what names
that other decoder uses.

The particular preamble that you pointed out would even be necessary
for on the fly patch construction.

>> >> >> +/// 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.
>> >
>> > gcc does not complain if all your variables are marked properly
>> > it just complains if a subset is marked const
>> >
>>
>> gcc believes that multilevel const casts are fundamentally unsafe
>>
>> /home/alex/Projects/ffmpeg/aac-sbr/ffmpeg/libavcodec/aacsbr.c:1357:
>> note: expected ?const float (*)[2]? but argument is of type ?float
>> (*)[2]?
>
> can you show the code that triggers this warning?
>

sbr.h:
 165     ///Zeroth coefficient used to filter the subband signals
 166     float              alpha0[64][2];
 167     ///First coefficient used to filter the subband signals
 168     float              alpha1[64][2];

aacsbr.c:
1740         sbr_hf_gen(ac, sbr, sbr->X_high, sbr->X_low, sbr->alpha0,
sbr->alpha1,
1741                    sbr->data[ch].bw_array, sbr->data[ch].t_env,
1742                    sbr->data[ch].bs_num_env[1]);

1352 static int sbr_hf_gen(AACContext *ac, SpectralBandReplication *sbr,
1353                       float X_high[64][40][2], const float
X_low[32][40][2],
1354                       const float (*alpha0)[2], const float (*alpha1)[2],
1355                       const float bw_array[5], const uint8_t *t_env,
1356                       int bs_num_env)



>> >> >> + ? ?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.
>> >
>> > I thought more of writting it along the lines of:
>> > -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;
>> > +Y[1][i][m + kx][idx] += sbr->s_m[L][m] * phi_sign;
>> >
>>
>> I don't follow
>
> the code does either
> Y[1][i][m + kx][0] += sbr->s_m[L][m] * 0;
> Y[1][i][m + kx][1] += sbr->s_m[L][m] * 1 * phi_sign;
>
> or
>
> Y[1][i][m + kx][0] += sbr->s_m[L][m] * 0;
> Y[1][i][m + kx][1] += sbr->s_m[L][m] * -1 * phi_sign;
>
> or
>
> Y[1][i][m + kx][0] += sbr->s_m[L][m] * 1;
> Y[1][i][m + kx][1] += sbr->s_m[L][m] * 0 * phi_sign;
>
> or
>
> Y[1][i][m + kx][0] += sbr->s_m[L][m] * -1;
> Y[1][i][m + kx][1] += sbr->s_m[L][m] * 0 * phi_sign;
>
> thus one is always a "no operation"
> you only need to do the other, and the +-1 can likely be merged into phi_sign
>

I see