[FFmpeg-devel] [PATCH] AAC Decoder round 4

[Michael Niedermayer]

On Wed, Jul 30, 2008 at 07:24:44PM +0100, Robert Swain wrote:
> 2008/7/30 Michael Niedermayer <michaelni at gmx.at>:
[...]

> 
> >> > [...]
> >> >> >> +            }
> >> >> >> +        }
> >> >> >> +    }
> >> >> >> +    return 0;
> >> >> >> +}
> >> >> >> +
> >> >> >> +/**
> >> >> >> + * Decode scale_factor_data; reference: table 4.47.
> >> >> >> + *
> >> >> >
> >> >> >> + * @param   mix_gain            channel gain (Not used by AAC bitstream.)
> >> >> >> + * @param   global_gain         first scalefactor value as scalefactors are differentially coded
> >> >> >> + * @param   band_type           array of the band type used for a window group's scalefactor band
> >> >> >
> >> >> >> + * @param   band_type_run_end   array of the last scalefactor band of a band type run for a window group's scalefactor band
> >> >> >
> >> >> > this sounds a little confusing
> >> >>
> >> >> Someone else (Diego?) said this was confusing but I'm not sure how to
> >> >> simplify it and keep the same information present. I wanted to clarify
> >> >> what the indices of the variable were. At the time of complaint I
> >> >> suggested something like "array of the last scalefactor band of a band
> >> >> type run with indices [window group][scalefactor band]"
> >> >
> >> > well then add an example to calrify what the 2 arrays contain
> >>
> >> I'm not really sure what to suggest other than something like this:
> >
> > you need to work on the line breaks ...
> >
> >
> >>
> >> Index: aac.c
> >> ===================================================================
> >> --- aac.c     (revision 2930)
> >> +++ aac.c     (working copy)
> >> @@ -1052,8 +1052,10 @@
> >>  /**
> >>   * Decode section_data payload; reference: table 4.46.
> >>   *
> >> - * @param   band_type           array of the band type used for a
> >> window group's scalefactor band
> >> - * @param   band_type_run_end   array of the last scalefactor band of
> >> a band type run for a window group's scalefactor band
> >> + * @param   band_type           array of the used band type
> >> + * @param   band_type_run_end   array of the last scalefactor band of
> >> a band type run
> >> + *
> >
> > honestly, what are you doing? :)
> > disable the darn line wraping of your editor/mua or attach the patch please
> > i would review it but i honestly cannot read this
> 
> Ah, sorry. I was pasting in-line as for some reason I thought it would
> be easier to maintain context as you've been quoting the main patch
> and commenting on it. I forgot that GMail does word wrapping after the
> text has been written. See attached (01-band_types.diff).

looks ok


> 
> > [...]
> >>
> >> >> >> +    int i;
> >> >> >> +    int n = 1;
> >> >> >> +    int num_excl_chan = 7;
> >> >> >> +
> >> >> >> +    for (i = 0; i < 7; i++)
> >> >> >> +         ac->che_drc.exclude_mask[i] = get_bits1(gb);
> >> >> >> +
> >> >> >> +    while (n <= MAX_CHANNELS && num_excl_chan < MAX_CHANNELS - 7 && get_bits1(gb)) {
> >> >> >> +        ac->che_drc.additional_excluded_chns[n-1]=1;
> >> >> >> +        for (i = num_excl_chan; i < num_excl_chan+7; i++)
> >> >> >> +            ac->che_drc.exclude_mask[i] = get_bits1(gb);
> >> >> >> +        n++;
> >> >> >> +        num_excl_chan += 7;
> >> >> >> +    }
> >> >> >
> >> >> > the for and while can maybe be merged
> >> >>
> >> >> How about this?
> >> >
> >> > lin
> >> > e b
> >> > rea
> >> > ks
> >>
> >> :) Where? Does this help?:
> >
> > no, see yourself:
> >
> > [...]
> >> +         (((i+1)%7) || (ac->che_drc.additional_excluded_chns[n++] =
> >> get_bits1(gb)));
> >> +         i++)
> 
> Again, I thought you mean line-breaks in the code, not in how the
> patch was coming through in the e-mail. Again, see attached
> (20080728-1344-refactor_excluded_channels.diff).
[...]
> Index: aac.c
> ===================================================================
> --- aac.c	(revision 2918)
> +++ aac.c	(working copy)
> @@ -279,7 +279,7 @@
>      int dyn_rng_sgn[17];                            ///< DRC sign information; 0 - positive, 1 - negative
>      int dyn_rng_ctl[17];                            ///< DRC magnitude information
>      int exclude_mask[MAX_CHANNELS];                 ///< Channels to be excluded from DRC processing.
> -    int additional_excluded_chns[MAX_CHANNELS];     /**< The exclude_mask bits are
> +    int additional_excluded_chns[MAX_CHANNELS / 7]; /**< The exclude_mask bits are
>                                                          coded in groups of 7 with 1 bit preceeding each group (except the first)
>                                                          indicating that 7 more mask bits are coded. */
>      int band_incr;                                  ///< Number of DRC bands greater than 1 having DRC info.
> @@ -1607,21 +1607,13 @@
>   * @return  Returns number of bytes consumed.
>   */
>  static int decode_drc_channel_exclusions(AACContext * ac, GetBitContext * gb) {
> -    int i;
> -    int n = 1;
> -    int num_excl_chan = 7;
> +    int i, n;
>  
> -    for (i = 0; i < 7; i++)
> -         ac->che_drc.exclude_mask[i] = get_bits1(gb);
> +    for (i=0, n=0; i < MAX_CHANNELS && (((i+1)%7) || (ac->che_drc.additional_excluded_chns[n++] = get_bits1(gb))); i++)
> +        ac->che_drc.exclude_mask[i] = get_bits1(gb);

i think this is a little obfuscated

what i meant was more along the lines of:

do{
    for (i = 0; i < 7; i++)
        ac->che_drc.exclude_mask[i+X] = get_bits1(gb);
    X+= 7;
}while(get_bits1(gb));

(note this likely is not correct, its just to show what i meant)

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20080730/873c825f/attachment.pgp>