2009/9/26 Diego Biurrun <di...@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
_______________________________________________
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
