[FFmpeg-soc] [soc]: r5388 - in sbr-wip: . TODO aacsbr.c aacsbr.h aacsbrdata.h checkout.sh ffmpeg.diff

[Diego Biurrun]

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