[FFmpeg-devel] [PATCH] AAC Decoder - Round 2.

[Robert Swain]

2008/6/21 Michael Niedermayer <michaelni at gmx.at>:
> On Sat, Jun 21, 2008 at 04:28:23PM +0100, Robert Swain wrote:
>> 2008/6/20 Michael Niedermayer <michaelni at gmx.at>:
>> > [...]
>> >> +/**
>> >> + * Program config. This describes how channels are arranged.
>> >> + *
>> >> + * Either read from stream (ID_PCE) or created based on a default
>> >> + * fixed channel arrangement.
>> >> + */
>> >> +typedef struct {
>> >> +    int che_type[4][MAX_TAGID];   ///< Channel Element type with the first index the first 4 raw_data_block IDs
>> >> +
>> >> +    int mono_mixdown;         ///< The SCE tag to use if user requests mono output,   -1 if not available
>> >> +    int stereo_mixdown;       ///< The CPE tag to use if user requests stereo output, -1 if not available
>> >> +    int mixdown_coeff_index;  ///< 0-3
>> >> +    int pseudo_surround;      ///< Mix surround channels out of phase
>> >> +
>> >> +} program_config_struct;
>> >
>> > i would drop the _struct
>> > one can always write "struct program_config" or ProgramConfig
>> > other structs in lav* dont have struct in their names either ...
>>
>> I agree. I don't like the _struct suffix on _every_ struct. Would you
>> prefer migration of all structs to 'ProgramConfig' style or maybe
>
> yes
>
>
>> 'program_config_t' for typedef structs and 'program_config_s' for
>
> no, this one risks issues with posix ...

Is the attached OK? Do the names look acceptable to you?

> [...]
>> >> +
>> >> +// aux
>> >> +/**
>> >> + * Generate a sine Window.
>> >> + */
>> >> +static void sine_window_init(float *window, int n) {
>> >> +    const float alpha = M_PI / n;
>> >> +    int i;
>> >> +    for(i = 0; i < n/2; i++)
>> >> +        window[i] = sin((i + 0.5) * alpha);
>> >> +}
>> >
>> > greping for sine_window finds some matches in lavc, is this and the table
>> > duplicated?
>>
>> It is used in nellymoser and imc as an MDCT sine window. Should this
>> function be moved to mdct.c and implemented in these two (and any
>> others as appropriate) codecs?
>
> we should not have duplicate tables and duplicate inits.
> moving shared things to mdct.c is ok

OK. I'll make a patch for this.

> [...]
>> > [...]
>> >> +static void decode_ms_data(AACContext * ac, GetBitContext * gb, che_struct * cpe) {
>> >> +    ms_struct * ms = &cpe->ms;
>> >> +    int g, i;
>> >> +    ms->present = get_bits(gb, 2);
>> >> +    if (ms->present == 1) {
>> >> +        for (g = 0; g < cpe->ch[0].ics.num_window_groups; g++)
>> >> +            for (i = 0; i < cpe->ch[0].ics.max_sfb; i++)
>> >> +                ms->mask[g][i] = get_bits1(gb);// << i;
>> >> +    } else if (ms->present == 2) {
>> >> +        for (g = 0; g < cpe->ch[0].ics.num_window_groups; g++)
>> >
>> >> +            memset(ms->mask[g], 1, cpe->ch[0].ics.max_sfb);
>> >
>> > this is missing some sizeof()
>>
>> I think it's because ms->mask[][] is uint8_t. I can add *
>> sizeof(ms->mask[g][0]) if you like.
>
> please do

Done.

Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20080621-1804-rename_structs.diff
Type: text/x-diff
Size: 20595 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080621/9b4a13be/attachment.diff>