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

[Robert Swain]

2009/9/26 Diego Biurrun <diego at biurrun.de>:
> 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...

Thanks for doing it, though I expect the code to change quite a bit
before it gets close to hitting trunk.

>> 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.

Done.

>> --- /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)'.

Done.

>> +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.

I think you've done most (all?). Thank you.

>> +        if (array[i] < min) min = array[i];
>
> I'd break that line.

Done.

>> +int qsort_compare(const int *a, const int *b)
>> +{
>> +    return *a - *b;
>> +}
>
> I'd expect a function called qsort_compare to sort...

Renamed and fixed to avoid GCC warnings.

>> +                if (ch_data->bs_invf_mode[1][i] == 0) {
>
> Michael prefers '!exp' instead of 'exp == 0' in if conditions.

An old habit that soon died. Fixed anyway.

>> +            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.

I think you've done this. Thanks.

>> +        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

The braces aren't useless according to you. :) They keep GCC quiet
about suggested parentheses around assignment in logical expressions.

I think you fixed the indentation - thanks.

>> +            sbr->gain_max[l][m] = gain_max_temp[l][k];
>> +            sbr->q_m_lim[l][m] = FFMIN(sbr->q_m[l][m],
>
> align

Again, you've done this.

>> +static void apply_sbr()
>> +{
>> +#if 0
>
> Why is this disabled?

Because it's a very early draft of the function and I wanted to check
everything else compiled properly.

>> +#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.

Agreed. Thank you for seeing to it.

>> +#define T_HFADJ 2
>
> That name should IMO be explained.

Will do, not done yet.

>> --- /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.

Done.

>> --- /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

Will do, not done yet.

>> +@@ -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..

Mmm, I'll sort that.

> I'll fix some of the issues I noticed myself in a moment.

Thanks.

Regards,
Rob