On Sat, Sep 26, 2009 at 04:59:27PM +0200, superdump wrote:
>
> Log:
> Initial AAC Spectral Band Replication commit. Please note that this code is
> not
> yet functional and needs cleaning up quite a bit.
\o/
Here's my initial review...
> Added:
> sbr-wip/
> sbr-wip/TODO
> sbr-wip/aacsbr.c
> sbr-wip/aacsbr.h
> sbr-wip/aacsbrdata.h
> sbr-wip/checkout.sh (contents, props changed)
> sbr-wip/ffmpeg.diff
I'd just call it aac-sbr, if you hadn't told me wip meant work in
progress, I would not have guessed. Also, all SoC code is work in
progress.
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ sbr-wip/aacsbr.c Sat Sep 26 16:59:27 2009 (r5388)
> @@ -0,0 +1,1509 @@
> +av_cold void ff_aac_sbr_init()
Functions without parameters should be declared as 'int foo(void)'.
> +int array_min_int(int *array, int nel)
> +{
> + int i, min = array[0];
> + for (i=1; i<nel; i++)
Spaces around operators would help readability here and in all other for
statements.
> + if (array[i] < min) min = array[i];
I'd break that line.
> +int qsort_compare(const int *a, const int *b)
> +{
> + return *a - *b;
> +}
I'd expect a function called qsort_compare to sort...
> + if (ch_data->bs_invf_mode[1][i] == 0) {
Michael prefers '!exp' instead of 'exp == 0' in if conditions.
> + gain_max_temp[l][k] = limgain[sbr->bs_limiter_gains]
> + * FFMIN(100000, sqrtf((EPS0 + sum[0]) / (EPS0 + sum[1])));
The * should rather be aligned with limgain.
> + for (m=0; m<sbr->m; m++) {
> + if ((k = find_freq_subband(sbr->f_tablelim, sbr->n_lim, m +
> sbr->k[3])) < 0) {
> + av_log(ac->avccontext, AV_LOG_ERROR,
> + "No subband found for frequency %d\n", m + sbr->k[3]);
> + }
indentation, useless braces
> + sbr->gain_max[l][m] = gain_max_temp[l][k];
> + sbr->q_m_lim[l][m] = FFMIN(sbr->q_m[l][m],
align
> +static void apply_sbr()
> +{
> +#if 0
Why is this disabled?
> +#include <stdint.h>
> +
> +#define SBR_INIT_VLC_STATIC(num, size) \
> + INIT_VLC_STATIC(&vlc_sbr[num], 9,
> sbr_tmp[num].table_size/sbr_tmp[num].elem_size, \
Spaces around / would help readability here.
> +#define T_HFADJ 2
That name should IMO be explained.
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ sbr-wip/checkout.sh Sat Sep 26 16:59:27 2009 (r5388)
> @@ -0,0 +1,9 @@
> +#! /bin/sh
Leave out the space.
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ sbr-wip/ffmpeg.diff Sat Sep 26 16:59:27 2009 (r5388)
> +@@ -101,7 +103,11 @@
> +
> ++extern av_cold void ff_aac_sbr_init();
> ++extern int ff_decode_sbr_extension(AACContext *ac, SpectralBandReplication
> *sbr,
> ++ GetBitContext *gb, int crc, int cnt, int
> id_aac);
indentation
> +@@ -1377,7 +1369,8 @@
> + *
> + * @return Returns number of bytes consumed
> + */
> +-static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int
> cnt)
> ++static int decode_extension_payload(AACContext * ac, GetBitContext * gb,
> ++ int cnt, int id, int tag)
cosmetics and bad ones to boot..
I'll fix some of the issues I noticed myself in a moment.
Diego
_______________________________________________
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
