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

Michael Niedermayer michaelni
Sat Jun 21 18:39:54 CEST 2008


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

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


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


[...]
> 
> >> +                        int j;
> >> +                        if (index == -1) {
> >> +                            av_log(ac->avccontext, AV_LOG_ERROR, "Error in spectral data\n");
> >> +                            return -1;
> >> +                        }
> >
> > can that happen at all?
> 
> Unless I misunderstand, it doesn't look like it as all the VLC tables
> are unsigned. Remove?

ehhh
-1 can happen if you have a vlc table with codes like
01
110
111
10

and you receive 00

it cannot happen with
01
001
000
110
111
10

to check this for random tables just add the 2^-len of all codes
4*2^-3 + 2*2^-2 = 1 here and the previous table is < 1
If theres one <1 then -1 can happen and should be checked otherwise
it is redundant.

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20080621/a5e48975/attachment.pgp>



More information about the ffmpeg-devel mailing list