[FFmpeg-devel] [PATCH 2/4] Add functions which are specific to E-AC-3 decoding.

[Michael Niedermayer]

On Sat, Jul 19, 2008 at 07:50:26PM -0400, justin.ruggles at gmail.com wrote:
[...]

> @@ -91,7 +91,7 @@ typedef struct {
>  
>  ///@defgroup aht adaptive hybrid transform
>      int channel_uses_aht[AC3_MAX_CHANNELS];                 ///< channel AHT in use     (chahtinu)
> -    int pre_mantissa[6][AC3_MAX_CHANNELS][AC3_MAX_COEFS];   ///< pre-IDCT mantissas
> +    int pre_mantissa[AC3_MAX_CHANNELS][AC3_MAX_COEFS][6];   ///< pre-IDCT mantissas

ok, and seperate commit


>  ///@}
>  
>  ///@defgroup channel channel
> @@ -172,4 +172,12 @@ typedef struct {
>  ///@}
>  } AC3DecodeContext;
>  
> +void ff_eac3_get_transform_coeffs_aht_ch(AC3DecodeContext *s, int ch);
> +
> +void ff_eac3_idct_transform_coeffs_ch(AC3DecodeContext *s, int ch, int blk);
> +
> +int ff_eac3_parse_header(AC3DecodeContext *s);
> +
> +void ff_eac3_tables_init(void);

As this adds functions seperate from their use. It would be nice to know why
they are needed? (as i cannot check without looking at seperate patches what
and where they are used an why they are non static ...)

Besides, idependant of that i think they should have some doxy comments

[...]
> +static int gaq_ungroup_tab[32][3];

is the function to init this smaller than the table?
is int needed or can a smaller data type be used?


> +
> +/** lrint(M_SQRT2*cos(2*M_PI/12)*(1<<23)) */
> +#define COEFF_0 10273905
> +
> +/** lrint(M_SQRT2*cos(0*M_PI/12)*(1<<23)) = lrint(M_SQRT2*(1<<23)) */
> +#define COEFF_1 11863283
> +
> +/** lrint(M_SQRT2*cos(5*M_PI/12)*(1<<23)) */
> +#define COEFF_2  3070444
> +
> +/**
> + * Calculate 6-point IDCT of the pre-mantissas.
> + * All calculations are 24-bit fixed-point.
> + */
> +static void idct6(int pre_mant[6])
> +{
> +    int tmp;

> +    int even[3], odd[3];

I wouldnt use arrays for intermediate variables, i doubt the compiler
is very good at putting their elements in registers


> +
> +    odd[1] = pre_mant[1] - pre_mant[3] - pre_mant[5];
> +
> +    even[2] = ((int64_t) pre_mant[2]                * COEFF_0) >> 23;
> +    tmp     = ((int64_t) pre_mant[4]                * COEFF_1) >> 23;
> +    odd[0]  = ((int64_t)(pre_mant[1] + pre_mant[5]) * COEFF_2) >> 23;
> +
> +    even[0] = pre_mant[0] + (tmp >> 1);
> +    even[1] = pre_mant[0] - tmp;
> +
> +    tmp = even[0];
> +    even[0] = tmp + even[2];
> +    even[2] = tmp - even[2];
> +
> +    tmp = odd[0];
> +    odd[0] = tmp + pre_mant[1] + pre_mant[3];
> +    odd[2] = tmp + pre_mant[5] - pre_mant[3];


LL constants to avoid the casts

/** lrint(M_SQRT2*cos(2*M_PI/12)*(1<<23)) */
#define COEFF_0 10273905LL

/** lrint(M_SQRT2*cos(0*M_PI/12)*(1<<23)) = lrint(M_SQRT2*(1<<23)) */
#define COEFF_1 11863283LL

/** lrint(M_SQRT2*cos(5*M_PI/12)*(1<<23)) */
#define COEFF_2  3070444LL


slightly reordered to avoid a few useless assignments

even[2] = (pre_mant[2] * COEFF_0) >> 23;
tmp     = (pre_mant[4] * COEFF_1) >> 24;
even[1] = pre_mant[0] - 2*tmp;
tmp    += pre_mant[0];
even[0] = tmp + even[2];
even[2] = tmp - even[2];


odd[1] =   pre_mant[1] - pre_mant[3] - pre_mant[5];
tmp    = ((pre_mant[1]               + pre_mant[5]) * COEFF_2) >> 23;
odd[0] = tmp + pre_mant[1] + pre_mant[3];
odd[2] = tmp + pre_mant[5] - pre_mant[3];


[...]
> +        // TODO: modify AC3 demuxer to allow user to select which substream to decode

Iam not sure how to support substreams but that is likey not the ideal solution
decoding all substreams and reencoding and storing them as seperates streams
must be possible ...


[...]
> +    s->block_switch_syntax = get_bits1(gbc);
> +    if (!s->block_switch_syntax)
> +        memset(s->block_switch, 0, sizeof(s->block_switch));

this flag does not seem used anywhere except here thus does not need to
be in the context


> +
> +    s->dither_flag_syntax = get_bits1(gbc);
> +    if (!s->dither_flag_syntax) {

same


> +        s->dither_all = 1;
> +        for(ch = 1; ch <= s->fbw_channels; ch++)
> +            s->dither_flag[ch] = 1;
> +    }
> +    s->dither_flag[CPL_CH] = s->dither_flag[s->lfe_ch] = 0;
> +

> +    s->bit_allocation_syntax = get_bits1(gbc);
> +    if (!s->bit_allocation_syntax) {

and this as well

either this is very poorly written or VERY bad split in 2 patches.
its hard to review code that is incomplete or unused.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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-devel/attachments/20080720/2290b4d2/attachment.pgp>