[FFmpeg-devel] [PATCH] AAC Decoder round 4
Robert Swain
robert.swain
Wed Jul 30 18:10:42 CEST 2008
2008/7/30 Michael Niedermayer <michaelni at gmx.at>:
> 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
Done.
> [...]
>> >> + *
>> >> + * @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
OK. Done.
> [...]
>> >> + 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.
So how should this be handled? I apologise if this is a stupid
question. Should the values be assigned to temporary local vars and
written to the context after validation? Should this be done just for
max_sfb and num_swb or are there others?
> [...]
>> >> + }
>> >> + }
>> >> + }
>> >> + 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:
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
+ *
+ * The band_type* arrays have indices [window group][scalefactor band]
* @return Returns error status. 0 - OK, !0 - error
*/
static int decode_band_types(AACContext * ac, GetBitContext * gb,
IndividualChannelStream * ics, enum BandType band_type[][64], int
band_type_run_end[][64]) {
@@ -1092,9 +1094,11 @@
*
* @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
+ * @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
* @param sf array of scalefactors or intensity
stereo positions used for a window group's scalefactor band
+ *
+ * The band_type* arrays have indices [window group][scalefactor band]
* @return Returns error status. 0 - OK, !0 - error
*/
static int decode_scalefactors(AACContext * ac, GetBitContext * gb,
float mix_gain, unsigned int global_gain,
@@ -1246,8 +1250,10 @@
/**
* Decode spectral data; reference: table 4.50.
*
- * @param band_type array of the band type used for a window
group's scalefactor band
+ * @param band_type array of the used band type
* @param icoef array of quantized spectral data
+ *
+ * The band_type array has indices [window group][scalefactor band]
* @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]) {
@@ -1326,9 +1332,11 @@
* Dequantize and scale spectral data; reference: 4.6.3.3.
*
* @param icoef array of quantized spectral data
- * @param band_type array of the band type used for a window
group's scalefactor band
+ * @param band_type array of the used band type
* @param sf array of scalefactors or intensity stereo
positions used for a window group's scalefactor band
* @param coef array of dequantized, scaled spectral data
+ *
+ * The band_type array has indices [window group][scalefactor band]
*/
static void dequant(AACContext * ac, const IndividualChannelStream *
ics, const int icoef[1024],
const enum BandType band_type[][64], const float sf[][64],
float coef[1024]) {
> [...]
>> >> +/**
>> >> + * 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()
Done.
>> >> + 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?:
@@ -1607,21 +1607,16 @@
* @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);
- 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;
- }
- return n;
+ skip_bits(gb, 7 - (i + !(i == MAX_CHANNELS)) % 7);
+ return n + 1;
}
/**
>> 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
Done.
> [...]
>> > [...]
>> >> +/**
>> >> + * 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
I prefer the shorter variant. Done for both.
> [...]
>
>>
>> >> +
>> >> +/**
>> >> + * 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!
Done.
>> > and the oter parameters should be documented as well
>>
>> Is it really necessary to document ac and cc?
>
> no
Didn't think so. :)
> [...]
>> > [...]
>> >> +/**
>> >> + * 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.
Done.
Regards,
Rob
More information about the ffmpeg-devel
mailing list