[FFmpeg-soc] LC AAC
Michael Niedermayer
michaelni at gmx.at
Sat Dec 15 18:26:44 CET 2007
On Sat, Dec 15, 2007 at 04:00:08PM +0000, Robert Swain wrote:
> Hello,
>
> I would like to request a review of the current LC AAC code.
i do not think the code in soc/aac is ready for a review yet, but as you
asked heres a quick one
> //tns
> #define TNS_MAX_ORDER 20
comment useless, not doxygen compatible, what is TNS? and a license header is
missing
>
> // sampling table
> static const int sampling_table[] = { 96000, 88200, 64000, 48000, 44100, 32000, 24000, 22050, 16000, 12000, 11025, 8000, 7350 };
duplicate of aac_sample_rates
[...]
> static const uint16_t swb_offset_128_96[] = {
> 0, 4, 8, 12, 16, 20, 24, 32, 40, 48, 64, 92, 128
> };
>
> static const uint16_t swb_offset_1024_64[] = {
> 0, 4, 8, 12, 16, 20, 24, 28, 32, 36, 40, 44, 48, 52, 56,
> 64, 72, 80, 88, 100, 112, 124, 140, 156, 172, 192, 216, 240, 268,
> 304, 344, 384, 424, 464, 504, 544, 584, 624, 664, 704, 744, 784, 824,
> 864, 904, 944, 984, 1024
> };
>
> static const uint16_t swb_offset_128_64[] = {
> 0, 4, 8, 12, 16, 20, 24, 32, 40, 48, 64, 92, 128
> };
duplicate, and there are more, find and remove all duplicates!
[...]
> // SSR table
> static float ssr_q_table[] = {
useless comment
[...]
> // Huffman tables
> static const unsigned int aac_scalefactor_huffman_table[][2] = {
> /* codeword, code length */
> { 0x3FFE8, 18 },
table must be split, half fits in unt8_t and comment is useless, so is
aac_ prefix for a static table in aac*.h
[...]
>
> #define V_DEBUG
remove this
[...]
> #ifndef V_DEBUG
> #define AV_DEBUG(...)
> #endif
and this
[...]
> typedef struct {
> int present;
> int generated;
>
> int num_channels;
>
> int num_front;
> int front_cpe;
> int front_tag[MAX_TAGID];
>
> int num_side;
> int side_cpe;
> int side_tag[MAX_TAGID];
>
> int num_back;
> int back_cpe;
> int back_tag[MAX_TAGID];
>
> int num_lfe;
> int lfe_tag[MAX_TAGID];
>
> int num_assoc_data;
> int assoc_data_tag[MAX_TAGID];
>
> int num_cc;
> int cc_ind_sw;
> int cc_tag[MAX_TAGID];
>
> int mono_mixdown;
> int stereo_mixdown;
> int matrix_mixdown;
> int pseudo_surround;
> } program_config_struct;
>
> enum {
> MIXMODE_DEFAULT = 0,
> MIXMODE_1TO1,
> MIXMODE_2TO1,
> MIXMODE_1TO2,
> MIXMODE_2TO2,
> MIXMODE_MATRIX1,
> MIXMODE_MATRIX2,
> MIXMODE_UNKNOWN
> };
>
> typedef struct {
> int mode;
> int c_tag;
> int lr_tag;
> int sur_tag;
> float sce_gain[MAX_TAGID];
> float cpe_gain[MAX_TAGID][2];
> float lfe_gain[MAX_TAGID];
> } mix_config_struct;
>
> typedef struct {
> int present;
> int lag;
> float coef;
> int used[MAX_LTP_LONG_SFB];
> } ltp_struct;
>
> typedef struct {
> int intensity_present;
> int noise_present;
>
> int max_sfb;
> int window_sequence;
> int window_shape;
> int window_shape_prev;
> int predictor;
> int num_window_groups;
> uint8_t grouping;
> uint8_t group_len[8];
> // ltp
> ltp_struct ltp;
> ltp_struct ltp2;
> // calculated
> const uint16_t *swb_offset;
> int num_swb;
> int num_windows;
> int tns_max_bands;
> } ics_struct;
>
> typedef struct {
> int present;
> int n_filt[8];
> int length[8][4];
> int direction[8][4];
> int order[8][4];
> float *tmp2_map[8];
> int coef[8][4][TNS_MAX_ORDER];
> } tns_struct;
>
> typedef struct {
> int present;
> int mask[8][64];
> } ms_struct;
>
> // dynamic range compression
> typedef struct {
> int pce_instance_tag;
> int drc_tag_reserved_bits;
> int dyn_rng_sgn[17];
> int dyn_rng_ctl[17];
> int exclude_mask[MAX_CHANNELS];
> int additional_excluded_chns[MAX_CHANNELS];
> int drc_band_incr;
> int drc_interpolation_scheme;
> int drc_band_top[17];
> int prog_ref_level;
> int prog_ref_level_reserved_bits;
> } drc_struct;
>
> typedef struct {
> int present;
> int num_pulse;
> int start;
> int offset[4];
> int amp[4];
> } pulse_struct;
>
> typedef struct {
> int max_band;
> int adjust_num[4][8];
> int alev[4][8][8];
> int aloc[4][8][8];
> float buf[4][24];
> } ssr_struct;
>
> typedef struct {
> int ind_sw;
> int domain;
>
> int num_coupled;
> int is_cpe[9];
> int tag_select[9];
> int l[9];
> int r[9];
>
> float gain[18][8][64];
> } coupling_struct;
>
> // individual channel element
> typedef struct {
> int global_gain;
> float mixing_gain;
> ics_struct ics;
> tns_struct tns;
> int cb[8][64]; // codebooks
> float sf[8][64];
> DECLARE_ALIGNED_16(float, coeffs[1024]);
> DECLARE_ALIGNED_16(float, saved[1024]);
> DECLARE_ALIGNED_16(float, ret[1024]);
> int16_t *ltp_state;
> ssr_struct *ssr;
> } sce_struct;
>
> // channel element
> typedef struct {
> int common_window;
> ms_struct ms;
> sce_struct ch[2];
> } cpe_struct;
>
> typedef struct {
> coupling_struct coup;
> sce_struct ch;
> } cc_struct;
>
> typedef struct {
> float q[4][4];
> float t0[4][12];
> float t1[4][12];
> } ssr_context;
>
> typedef struct {
> // objects
> AVCodecContext * avccontext;
> GetBitContext gb;
> VLC mainvlc;
> VLC books[11];
>
> // main config
> int audioObjectType;
> int ext_audioObjectType;
> int sbr_present;
> int sampling_index;
> int ext_sampling_index;
> int sample_rate;
> int ext_sample_rate;
> int channels;
> int frame_length;
>
> // decoder param
> program_config_struct pcs;
> mix_config_struct mix;
> sce_struct * che_sce[MAX_TAGID];
> cpe_struct * che_cpe[MAX_TAGID];
> sce_struct * che_lfe[MAX_TAGID];
> cc_struct * che_cc[MAX_TAGID];
> drc_struct * che_drc;
>
> DECLARE_ALIGNED_16(float, buf_mdct[2048]);
> int is_saved;
>
> //cashes
> const uint16_t *swb_offset_1024;
> const uint16_t *swb_offset_128;
> int num_swb_1024;
> int num_swb_128;
> int tns_max_bands_1024;
> int tns_max_bands_128;
>
> // tables
> DECLARE_ALIGNED_16(float, kbd_long_1024[1024]);
> DECLARE_ALIGNED_16(float, kbd_short_128[128]);
> DECLARE_ALIGNED_16(float, sine_long_1024[1024]);
> DECLARE_ALIGNED_16(float, sine_short_128[128]);
> DECLARE_ALIGNED_16(float, pow2sf_tab[256]);
> DECLARE_ALIGNED_16(float, intensity_tab[256]);
> DECLARE_ALIGNED_16(float, ivquant_tab[256]);
> DECLARE_ALIGNED_16(float, revers[1024]);
> float* interleaved_output;
> float* iop;
>
> MDCTContext mdct;
> MDCTContext mdct_small;
> MDCTContext *mdct_ltp;
> DSPContext dsp;
> int * vq[11];
> ssr_context * ssrctx;
> AVRandomState random_state;
>
> //bias values
> int add_bias;
> int scale_bias;
>
> // statistics
> int num_frame;
> } AACContext;
if anything in the above is unused or duplicated or can be done with local
variables, remove it!
[...]
> static inline int16_t LTP_ROUND(float x) {
> if (x >= 0)
> {
> if (x >= 1.0f)
> return 32767;
> } else {
> if (x <= -1.0f)
> return -32768;
> }
>
> return lrintf(32768 * x);
> }
inefficient.
use dsputil! and uppercase function name is unacceptable
[...]
> /**
> * Generate a Kaiser-Bessel Derived Window.
> * @param alpha determines window shape
> * @param window array to fill with window values
> * @param n length of the window
> * @param iter number of iterations to use in BesselI0
> */
> static void kbd_window_init(int alpha, float *window, int n, int iter) {
> int k, n2;
> float *kwindow;
>
> n2 = n >> 1;
> kwindow = &window[n2];
> k_window_init(alpha, kwindow, n2, iter);
> window[0] = kwindow[0];
> for(k=1; k<n2; k++) {
> window[k] = window[k-1] + kwindow[k];
> }
> for(k=0; k<n2; k++) {
> window[k] = sqrt(window[k] / (window[n2-1]+1));
> //window[n-1-k] = window[k];
> }
> }
duplicated
[...]
> ac->sampling_index = get_bits(gb, 4);
> assert(ac->sampling_index <= 12);
> ac->sample_rate = sampling_table[ac->sampling_index];
> pcs->num_front = get_bits(gb, 4);
> pcs->num_side = get_bits(gb, 4);
> pcs->num_back = get_bits(gb, 4);
> pcs->num_lfe = get_bits(gb, 2);
> pcs->num_assoc_data = get_bits(gb, 3);
> pcs->num_cc = get_bits(gb, 4);
vertical align, invalid use of assert()
>
> pcs->mono_mixdown = get_bits1(gb) ? get_bits(gb, 4) + 1: 0;
> pcs->stereo_mixdown = get_bits1(gb) ? get_bits(gb, 4) + 1: 0;
> assert(pcs->mono_mixdown == 0 && pcs->stereo_mixdown == 0);
invalid use of assert()
[...]
> pcs->front_cpe = 0;
> ac->channels += pcs->num_front;
> for (i = 0; i < pcs->num_front; i++) {
> if (get_bits1(gb)) {
> pcs->front_cpe |= (1 << i);
> ac->channels++;
> }
> pcs->front_tag[i] = get_bits(gb, 4);
> }
> pcs->side_cpe = 0;
> ac->channels += pcs->num_side;
> for (i = 0; i < pcs->num_side; i++) {
> if (get_bits1(gb)) {
> pcs->side_cpe |= (1 << i);
> ac->channels++;
> }
> pcs->side_tag[i] = get_bits(gb, 4);
> }
> pcs->back_cpe = 0;
> ac->channels += pcs->num_back;
> for (i = 0; i < pcs->num_back; i++) {
> if (get_bits1(gb)) {
> pcs->back_cpe |= (1 << i);
> ac->channels++;
> }
> pcs->back_tag[i] = get_bits(gb, 4);
> }
code triplification
[...]
> skip_bits(gb, 8 * get_bits(gb, 8));
invalid, skip bits does not support arbitrary lengths
end of review, this makes no sense, iam not willing to do your work, read
through the code and clean it up before asking for a review
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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-soc/attachments/20071215/460fadab/attachment.pgp>
More information about the FFmpeg-soc
mailing list