[FFmpeg-devel] [PATCH] AAC Decoder round 4
Michael Niedermayer
michaelni
Wed Jul 30 17:14:35 CEST 2008
On Wed, Jul 30, 2008 at 01:41:54PM +0100, Robert Swain wrote:
[...]
> >> +
> >> +/**
> >> + * Decode a data_stream_element; reference: table 4.10.
> >> + */
> >> +static void data_stream_element(AACContext * ac, GetBitContext * gb) {
> >
> > this looks at best like a skip_something()
>
> Indeed it does. It seems strange to me that a bit stream should
> contain such useless bytes but I guess they are (or may be) actually
> used for something. I could make it skip_data_stream_element().
yes skip_data_stream_element() is a much better name
[...]
>
> >> + *
> >> + * @param common_window Channels have independent [0], or shared [1], Individual Channel Stream information.
> >> + */
> >> +static int decode_ics_info(AACContext * ac, GetBitContext * gb, int common_window, IndividualChannelStream * ics) {
> >
> >> + uint8_t grouping;
> >
> > why uint8_t ?
>
> Because the variable is an unsigned integer and is 7 bits read from
> the bit stream. It can be unsigned int or int as you please. I don't
> mind.
i prefer unsigned int unless we do need a 8 bit twos complement variable
[...]
> >> + ics->num_window_groups = 1;
> >> + ics->group_len[0] = 1;
> >> + if (ics->window_sequence == EIGHT_SHORT_SEQUENCE) {
> >> + int i;
> >> + ics->max_sfb = get_bits(gb, 4);
> >> + grouping = get_bits(gb, 7);
> >> + for (i = 0; i < 7; i++) {
> >> + if (grouping & (1<<(6-i))) {
> >> + ics->group_len[ics->num_window_groups-1]++;
> >> + } else {
> >> + ics->num_window_groups++;
> >> + ics->group_len[ics->num_window_groups-1] = 1;
> >> + }
> >> + }
> >> + ics->swb_offset = swb_offset_128[ac->m4ac.sampling_index];
> >> + ics->num_swb = num_swb_128[ac->m4ac.sampling_index];
> >
> >> + if(ics->max_sfb > ics->num_swb) {
> >> + av_log(ac->avccontext, AV_LOG_ERROR,
> >> + "Number of scalefactor bands in group (%d) exceeds limit (%d).\n",
> >> + ics->max_sfb, ics->num_swb);
> >> + return -1;
> >> + }
> >
> > is it safe to write invalid values in the context and then exit with an
> > error? are they gurranteed not to be used or that their use is harmless?
>
> If this function returns -1 it will fall through to aac_decode_frame
> returning -1.
yes but i think it can end up being used in future frames without
the code above being reexecuted to clean it up.
These may be bugs elsewhere but still i dont like security related
code that depends on the absence of bugs in large amounts of
distant code.
[...]
> >> + int k = 0;
> >> + while (k < ics->max_sfb) {
> >
> > for(k=0; k < ics->max_sfb; ) {
>
> I'll still need 'int k;' unless the declaration is moved to be
> alongside that for 'g'.
hmm ok forget it
[...]
> >> + }
> >> + }
> >> + }
> >> + 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
[...]
> >> +}
> >> +
> >> +/**
> >> + * Decode spectral data; reference: table 4.50.
> >> + *
> >
> >> + * @param band_type array of the band type used for a window group's scalefactor band
> >> + * @param icoef array of quantized spectral data
> >> + * @return Returns error status. 0 - OK, !0 - error
> >> + */
> >> +static int decode_spectrum(AACContext * ac, GetBitContext * gb, const IndividualChannelStream * ics, const enum BandType band_type[][64], int icoef[1024]) {
> >
> > If you want to have all output arrays after the input thats fine as well
> > (if done consistently)
> > if not i would prefer them to be before the inputs.
>
> To me, reading left to right, in, in/out, out ordered by their use in
> the function or alphabetically or something seems most logical. I'll
> add it to my list and it will be done before resubmission.
in C
assignment goes
out = in;
not
in = out;
so the out at the left seems more natural to me, also functions like
memcpy have the out first.
Anyway this is nitpicking and not relevant, whats important is that its
at least done consistantly within a file.
[...]
> > [...]
> >> +/**
> >> + * intensity stereo decoding; reference: 4.6.8.2.3
> >> + */
> >> +static void apply_intensity_stereo(AACContext * ac, ChannelElement * cpe) {
> >> + const IndividualChannelStream * ics = &cpe->ch[1].ics;
> >> + SingleChannelElement * sce1 = &cpe->ch[1];
> >> + float *coef0 = cpe->ch[0].coeffs, *coef1 = cpe->ch[1].coeffs;
> >> + const uint16_t * offsets = ics->swb_offset;
> >> + int g, gp, i, k;
> >> + int c;
> >> + float scale;
> >> + for (g = 0; g < ics->num_window_groups; g++) {
> >> + for (gp = 0; gp < ics->group_len[g]; gp++) {
> >> + for (i = 0; i < ics->max_sfb; i++) {
> >
> >> + if (sce1->band_type[g][i] == INTENSITY_BT || sce1->band_type[g][i] == INTENSITY_BT2) {
> >> + c = -1 + 2 * (sce1->band_type[g][i] - 14);
> >
> > i think these could be by using band_type_run_end be moved out of the loop
> > but maybe its not worth speed vs complexity wise ?
>
> Done.
>
> > [...]
> >> +/**
> >> + * Decode a channel_pair_element; reference: table 4.4.
> >> + *
> >> + * @param tag Identifies the instance of a syntax element.
> >> + * @return Returns error status. 0 - OK, !0 - error
> >> + */
> >> +static int decode_cpe(AACContext * ac, GetBitContext * gb, int tag) {
> >> + int i, ret;
> >> + ChannelElement * cpe;
> >> +
> >> + cpe = ac->che[ID_CPE][tag];
> >
> >> + cpe->common_window = get_bits1(gb);
> >
> > This variable does not seem used outside this function thus could be a local
> > variable (and the one in the struct of course removed)
>
> Done.
>
> > [...]
> >> +/**
> >> + * Parse Spectral Band Replication extension data; reference: table 4.55.
> >> + *
> >> + * @param crc flag indicating the presence of CRC checksum
> >> + * @param cnt length of ID_FIL syntactic element in bytes
> >> + * @return Returns number of bytes consumed from the ID_FIL element.
> >> + */
> >
> >> +static int sbr_extension_data(AACContext * ac, GetBitContext * gb, int crc, int cnt) {
> >
> > The name is not good
>
> decode_sbr_extension() ?
>
> >> + // TODO : sbr_extension implementation
> >> + av_log(ac->avccontext, AV_LOG_DEBUG, "aac: SBR not yet supported.\n");
> >> + skip_bits_long(gb, 8*cnt - 4); // -4 due to reading extension type
> >> + return cnt;
> >> +}
> >> +
> >
> >> +/**
> >> + * Parse whether channels are to be excluded from Dynamic Range Compression; reference: table 4.53.
> >> + *
> >> + * @return Returns number of bytes consumed.
> >> + */
> >
> >> +static int excluded_channels(AACContext * ac, GetBitContext * gb) {
> >
> > this one neither
>
> Unless you would be happy with decode_excluded_channels(), which I
> think is ambiguous, I'm not sure what to suggest without adding extra
> grammar.
> decode_channels_to_be_excluded_from_drc()?
> decode_drc_channel_exclusions()?
both are better than excluded_channels()
>
> >> + 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
>
> 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
[...]
> I'm pretty sure it's right but it's untested. It would be handy if we
> had a set of _feature documented_ samples that allow testing of every
> feature/tool. As it is, I don't know which files use DRC to test, even
> if DRC is unused at the moment.
>
> >> + return n;
> >> +}
> >> +
> >> +/**
> >> + * Decode dynamic range information; reference: table 4.52.
> >> + *
> >> + * @param cnt length of ID_FIL syntactic element in bytes
> >> + * @return Returns number of bytes consumed.
> >> + */
> >
> >> +static int dynamic_range_info(AACContext * ac, GetBitContext * gb, int cnt) {
> >
> > This function name is not good
>
> decode_dynamic_range() ?
probably
[...]
> > [...]
> >> +/**
> >> + * Apply dependent channel coupling.
> >> + *
> >> + * @param index which gain to use for coupling
> >> + */
> >> +static void dependent_coupling(AACContext * ac, ChannelElement * cc, SingleChannelElement * sce, int index) {
> >
> > The doxy already has a pretty good name, the function though does not.
>
> apply_dependent_coupling() or apply_dependent_channel_coupling()?
> (Same for independent_coupling().)
all better than what its ATM
[...]
>
> >> +
> >> +/**
> >> + * channel coupling transformation interface
> >> + *
> >> + * @param index which gain to use for coupling
> >> + */
> >> +static void apply_channel_coupling(AACContext * ac, ChannelElement * cc,
> >> + void (*apply_coupling_method)(AACContext * ac, ChannelElement * cc, SingleChannelElement * sce, int index))
> >
> > how does a index select a gain?
>
> It seems that there is either one (L/R/shared) or two (L+R
> independent) gain_element lists coded for each CCE and these are
> parsed into ChannelCoupling.gain[index][][] with index being
> incremented with each list. So rather than being 'which gain to use'
> it should be 'index into gain array' or something like that.
great, write it in the doxy please!
>
> > and the oter parameters should be documented as well
>
> Is it really necessary to document ac and cc?
no
[...]
[...]
> > [...]
> >> +/**
> >> + * Conduct matrix mix-down and float to int16 conversion.
> >> + *
> >> + * @param data pointer to output data
> >> + * @param data_size output data size in bytes
> >> + * @return Returns error status. 0 - OK, !0 - error
> >> + */
> >> +static int output_samples(AVCodecContext * avccontext, uint16_t * data, int * data_size) {
> >
> > the name of the function and the 1 line description in the doxy do really
> > describe seperate things.
>
> mixdown_and_convert_to_int16()?
great, anything is better than these noun based function names.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/c3faa6ba/attachment.pgp>
More information about the ffmpeg-devel
mailing list