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

Michael Niedermayer michaelni
Mon Mar 8 11:01:26 CET 2010


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:
[...]
> >> >> +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
> 
> last_el(ement) has a sight semantic difference from length.
> last_el = length-1

i would have called it last_el_idx then 


> 
> But remove_table_element_int16 is no longer relevant



[...]
> 
> >
> > [...]
> >> >
> >> > [...]
> >> >> + ? ?// 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?
> >
> > "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"?


[...]
> 
> >> > [...]
> >> >> +/// 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.
> >
> > I surely can, but
> > as said this is unreasonable to expect from every reader of ffmpeg code.
> > and making the code less readable because of your stubbornness seems
> > quite unreasonable to me.
> > Please change l to some other identifer
> >
> 
> It isn't worth my time arguing this. Changed. Enjoy your 1980s fonts

thank you


[...]
> >
> >
> > [...]
> >> >
> >> >> + ? ?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.
> >
> > 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?


[...]
> >> > [...]
> >> >> + ? ?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


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100308/7dcf8a10/attachment.pgp>



More information about the ffmpeg-devel mailing list