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

[Justin Ruggles]

Michael Niedermayer wrote:
> On Sat, Jul 19, 2008 at 07:50:26PM -0400, justin.ruggles at gmail.com wrote:
> [...]
 >>  ///@}
>>  
>>  ///@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

So I guess this is the same thing as below with splitting commits.  One
large commit plus one cosmetic commit would show the use of these.

And I'll add 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?

int is not needed. I can't believe I missed that one. :)
As for the code size vs. table size, that I don't know.  With uint8_t,
the table size is 96 bytes.  If I move the table to ac3dec_data.c and
hardcode it, I can reuse it in the mantissa decoding, so I'm guessing it
would be smaller.

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

I tried both ways and it didn't affect speed on my machine.  I don't
have multiple machines to test with though.  I'll post some test results
of various changes so others can compare.

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

ok.

> 
> 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];

I'll test it out.  When I tried rearranging things to simplify the code,
it affected the speed significantly.

> [...]
>> +        // 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 ...

It didn't sound quite right to me either.  I can't think of any other
good way within the current framework.  Would it be ok to just add a
comment that we're skipping additional substreams instead of a TODO?  I
havn't found any samples for this anyway...

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

All those *_syntax variables are used in the decoder from within
ac3dec.c.  If I combine the 2 patches, it will be pretty large, but if
that's preferable then I can do it.

Thanks,
Justin