[FFmpeg-devel] [PATCH] HE-AAC v1 decoder

[Alex Converse]

On Wed, Jan 27, 2010 at 5:53 AM, Alexander Strange
<astrange at ithinksw.com> wrote:
>
> On Jan 26, 2010, at 10:10 PM, Alex Converse wrote:
>
>> Greetings Sirs,
>>
>> Enclosed you will find one (1) HE-AAC decoder to make glorious great
>> software FFmpeg.
>>
>> Heart,
>>
>> Alex
>
>
> Some comments:
>
>> @@ -1815,7 +1814,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);
>> ?}
>>
> Looks like a SIMD opportunity.
>

That's inside the AAC-LC/Main decoder. All I did here was correct the
impedance mismatch with SBR. SIMDing that should be done separately.

>> +/// constant to avoid division by zero, e.g. 96 dB below maximum signal input
>> +#define EPS0 0.000000000001
>
> You can use FLT_EPSILON.
>
>> +static void make_bands(int16_t* bands, int start, int stop, int num_bands)
>
> int16_t *bands
>
>> + ? ?base = powf((float)stop / start, 1.0f / num_bands);
>
> The (float) cast isn't needed.
> And the file uses 'float x = 1.0f' in a few places where the 'f' isn't needed, but it doesn't really matter.
>
>> + ? ?for (k = 0; k < num_bands-1; k++) {
>> + ? ? ? ?prod *= base;
>> + ? ? ? ?present ?= lroundf(start * prod);
>> + ? ? ? ?bands[k] = present - previous;
>> + ? ? ? ?previous = present;
>> + ? ?}
>
> You use lroundf() a lot, which is a library call for me. Can't you set the rounding direction (or just ignore it) and cast instead?
> Also, this is called later on with the output qsorted afterwards, but it doesn't look like it can be generated out of order.
>
>> + ? ?if (sbr->sample_rate == 16000) {
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[0];
>> + ? ?} else if (sbr->sample_rate == 22050) {
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[1];
>> + ? ?} else if (sbr->sample_rate == 24000) {
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[2];
>> + ? ?} else if (sbr->sample_rate == 32000) {
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[3];
>> + ? ?} else if ((sbr->sample_rate >= 44100) &&
>> + ? ? ? ? ? ? ? (sbr->sample_rate <= 64000)) {
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[4];
>> + ? ?} else if (sbr->sample_rate > 64000) {
>> + ? ? ? ?sbr_offset_ptr = sbr_offset[5];
>> + ? ?} else {
>> + ? ? ? ?av_log(ac->avccontext, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "Unsupported sample rate for SBR: %d\n", sbr->sample_rate);
>> + ? ? ? ?return -1;
>> + ? ?}
>
> Kind of long. I'd use a switch, except for the arbitrary samplerates allowed after 44.1k (why only there?).
>
>> + ? ? ? ?if (sbr->k[2] / (float)sbr->k[0] > 2.2449) {
>> + ? ? ? ? ? ?two_regions = 1;
>> + ? ? ? ? ? ?sbr->k[1] = sbr->k[0] << 1;
>> + ? ? ? ?} else {
>> + ? ? ? ? ? ?two_regions = 0;
>> + ? ? ? ? ? ?sbr->k[1] = sbr->k[2];
>> + ? ? ? ?}
>
> Here it should be 2.2449f. What's 'k'?
>
>> + ? ? ? ? ? ?if (vdk1_min < vdk0_max) {
>> + ? ? ? ? ? ? ? ?int change;
>> + ? ? ? ? ? ? ? ?qsort(vk1 + 1, num_bands_1, sizeof(vk1[1]), qsort_comparison_function_int16);
>> + ? ? ? ? ? ? ? ?change = FFMIN(vdk0_max - vk1[1], (vk1[num_bands_1] - vk1[1]) >> 1);
>> + ? ? ? ? ? ? ? ?vk1[1] ? ? ? ? ? += change;
>> + ? ? ? ? ? ? ? ?vk1[num_bands_1] -= change;
>> + ? ? ? ? ? ?}
>> +
>> + ? ? ? ? ? ?qsort(vk1 + 1, num_bands_1, sizeof(vk1[1]), qsort_comparison_function_int16);
>
> Looks like it unnecessarily sorts twice.
>
>> + ? ? ? ? ? ?sbr->n_master = num_bands_0 + num_bands_1;
>> + ? ? ? ? ? ?memcpy(&sbr->f_master[0], ? ? ? ? ? ? ? vk0,
>> + ? ? ? ? ? ? ? ? ? (num_bands_0 + 1) * sizeof(sbr->f_master[0]));
>> + ? ? ? ? ? ?memcpy(&sbr->f_master[num_bands_0 + 1], vk1 + 1,
>> + ? ? ? ? ? ? ? ? ? num_bands_1 ? ? ?* sizeof(sbr->f_master[0]));
>
> Nitpick: one more space before 'num_bands_1'.
>
>> + ? ?// 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;
>
> So name it that...
>
>> + ? ?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;
>> + ? ?}
>
> The formatting ("invalid bitstream") of this one is different from the other errors.
>
>> + ? ?if (goal_sb < sbr->k[4] + sbr->m[1]) {
>> + ? ? ? ?for (k = 0; sbr->f_master[k] < goal_sb; k++);
>> + ? ?} else
>> + ? ? ? ?k = sbr->n_master;
>
> IIRC some compilers warn about 'for (...);' with no space before ;.
>
>> +/// 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];
>> + ? ?int goal_sb = lroundf((1 << 11) * 1000 / (float)sbr->sample_rate);
>
> What do those names mean?

sb is definitely subband. Goal_sb is then goal subband. usb i think is
standing for upper subband. It's the first subband where SBR is
applied. msb is a mystery to me, perhaps master subband?

>
>> +static inline int in_table(void *table, int last_el, int el_size, void *needle)
>> +{
>> + ? ?int i;
>> + ? ?uint8_t *table_ptr = table; // avoids a warning with void * ptr arith
>> + ? ?for (i = 0; i <= last_el; i++, table_ptr += el_size)
>> + ? ? ? ?if (!memcmp(table_ptr, needle, el_size))
>> + ? ? ? ? ? ?return 1;
>> + ? ?return 0;
>> +}
>
> I think the comment is superfluous.
> Also, this is only used with sizeof(int16_t).
>
>> + ? ?// Limiter Frequency Band Table (14496-3 sp04 p198)
>> + ? ?if (sbr->bs_limiter_bands > 0) {
>> + ? ? ? ?const float lim_bands_per_octave[3] = {1.2, 2, 3};
>> + ? ? ? ?int16_t patch_borders[sbr->num_patches + 1];
>
> Not using 'static' might make it build the array on the stack anyway.
>
>> + ? ? ? ?dk = phi[2][1][0] * phi[1][0][0] -
>> + ? ? ? ? ? ? ? (phi[1][1][0] * phi[1][1][0] + phi[1][1][1] * phi[1][1][1]) / 1.000001f;
>
> * (1.f/1.000001f)
>
>> + ? ?if (sbr->bs_interpol_freq) {
>> + ? ? ? ?for (l = 0; l < ch_data->bs_num_env[1]; l++) {
>> + ? ? ? ? ? ?const int env_size = (ch_data->t_env[l + 1] - ch_data->t_env[l]) << 1;
>> ...
>> + ? ? ? ? ? ? ? ?e_curr[l][m] = sum / env_size;
>
> Use the inverse of env_size here too.
>
>> + ? ?// max gain limits : -3dB, 0dB, 3dB, inf dB (limiter off)
>> + ? ?static const float limgain[4] = { 0.70795, 1.0, 1.41254, 10000000000 };
>
> This is static const, but the ones in the next function aren't.
>
>> + ? ?float g_filt[42][48], q_filt[42][48], w_temp[42][48][2];
>> + ? ?float (*g_temp)[48] = ch_data->g_temp, (*q_temp)[48] = ch_data->q_temp;
>> + ? ?memcpy(Y[0], Y[1], sizeof(Y[0]));
>
> ...and they're 32KB+ on stack, maybe put them in a context?

I actually can eliminate w_temp and the outer dimensions from
q_filt/g_filt but forgot to.

>
> What is all the memcpy(x[0],x[1],sizeof(x[0])); used for?
>

To look back  the previous frame. I suppose I can switch circular addressing.

>> +void ff_sbr_dequant(AACContext *ac, SpectralBandReplication *sbr, int id_aac)
>> +{
>> + ? ?int ch;
>> +
>> + ? ?if (sbr->start) {
>> + ? ? ? ?for (ch = 0; ch < (id_aac == TYPE_CPE ? 2 : 1); ch++) {
>> + ? ? ? ?sbr_time_freq_grid(ac, sbr, &sbr->data[ch], ch);
>> + ? ? ? ?sbr_env_noise_floors(sbr, &sbr->data[ch], ch);
>> + ? ? ? ?}
>> + ? ? ? ?sbr_dequant(sbr, id_aac);
>> + ? ?}
>> +}
>
> 'if (!sbr->start) return;' if you want.
>
> And indent the loop contents.
>
>> + ? ?/**
>> + ? ? * @defgroup state ? ? ? ? State varaibles
>> + ? ? * @{
>> + ? ? */
>
> variables
>
> In SpectralBandReplication, you might get unnecessary struct padding if you put uint8_t in the middle of struct definitions. Best to put them at the end unless it's already organized for cache use.
>
>> --- /dev/null
>> +++ b/libavcodec/aacsbr_internal.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * AAC Spectral Band Replication function delcarations
>> + * Copyright (c) 2008-2009 Robert Swain ( rob opendot cl )
>
> declarations (and it's 2010)

I don't think Rob has written any code for it yet this year. My
copyright line is 2009-2010.

>
>> +/** Dequantized all channels in one SBR element. */
>> +void ff_sbr_dequant(AACContext *ac, SpectralBandReplication *sbr, int id_aac);
>
> dequantize
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>