[FFmpeg-devel] AAC decoder round 6
Michael Niedermayer
michaelni
Sun Aug 10 15:13:26 CEST 2008
On Sun, Aug 10, 2008 at 12:31:31PM +0100, Robert Swain wrote:
> 2008/8/9 Michael Niedermayer <michaelni at gmx.at>:
> > On Sat, Aug 09, 2008 at 12:25:09PM +0100, Robert Swain wrote:
> >> $subj
> > [...]
> >
> >> +const uint8_t ff_aac_num_swb_1024[] = {
> >> + 41, 41, 47, 49, 49, 51, 47, 47, 43, 43, 43, 40
> >> +};
> >> +
> >> +const uint8_t ff_aac_num_swb_128[] = {
> >> + 12, 12, 12, 14, 14, 14, 15, 15, 15, 15, 15, 15
> >> +};
> >> +
> >
> > dont these 2 belong to the swb offset tables? if so i think they should
> > be in the same file unless iam missing something either way these 2 are
> > ok [with static if they are only used by 1 file]
>
> Kostya wanted these two tables but not the rest of the swb tables so I
> made these shared and the others not. I can move them if you wish.
please add a note or move the tables
>
[...]
>
> > [...]
> >> +
> >> +/**
> >> + * Decode GA "General Audio" specific configuration; reference: table 4.1.
> >> + *
> >> + * @return Returns error status. 0 - OK, !0 - error
> >> + */
> >> +static int decode_ga_specific_config(AACContext * ac, GetBitContext * gb, int channel_config) {
> >> + enum ChannelPosition new_che_pos[4][MAX_ELEM_ID];
> >> + int extension_flag, ret;
> >> +
> >> + if(get_bits1(gb)) { // frameLengthFlag
> >> + av_log(ac->avccontext, AV_LOG_ERROR, "960/120 MDCT window is not supported.\n");
> >
> > is this "update to latest svn; sample welcome or patch welcome" ?
> > i think we have a function to print such messages, i dont remember what its
> > name was though
>
> Is that a joke? I don't really understand what you're trying to point
> out. It's more "These files don't really occur in the wild and we
> haven't written any support for them."
[FFmpeg-devel] [PATCH 1/4] add a generic function to lavc to log messages about missing features.
I approved the patch but it apparently was not applied
[...]
> >> + 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 = ff_aac_num_swb_128[ac->m4ac.sampling_index];
> >> + } else {
> >> + ics->max_sfb = get_bits(gb, 6);
> >> + ics->swb_offset = swb_offset_1024[ac->m4ac.sampling_index];
> >> + ics->num_swb = ff_aac_num_swb_1024[ac->m4ac.sampling_index];
> >> + }
> >
> > is there something that prevents sampling_index from being >=12 ?
> > a quick look at ff_mpeg4audio_get_config() indicates that it can at least
> > be 15.
>
> It's checked in decode_pce() but that is only called if the channel
> config changes. I've added another check after the call to
> ff_mpeg4audio_get_config() so now it is checked in both places where a
> sampling index is parsed.
>
> >> +
> >> + 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);
> >> + ics->max_sfb = 0;
> >> + ics->num_swb = 0;
> >> + return -1;
> >> + }
> >> +
> >
> >> + if (ics->window_sequence[0] == EIGHT_SHORT_SEQUENCE) {
> >> + ics->num_windows = 8;
> >> + ics->tns_max_bands = tns_max_bands_128[ac->m4ac.sampling_index];
> >> + } else {
> >> + ics->num_windows = 1;
> >> + ics->tns_max_bands = tns_max_bands_1024[ac->m4ac.sampling_index];
> >> + if (get_bits1(gb)) {
> >> + av_log(ac->avccontext, AV_LOG_ERROR,
> >> + "Predictor bit set but LTP is not supported.\n");
> >> + return -1;
> >> + }
> >> + }
> >
> > why is this split from the first part?
>
> It was split either side of the refactored max_sfb/num_swb validation.
> I can merge them but then if the max_sfb/num_swb check fails, should
> these values also be zeroed?
The way its done currently can leave the num_windows / tns_max_bands in
any state that was valid in the past combined with the other fields
in any independant state that was not valid now.
I did not check if this can cause problems ...
But if its left the code needs to be reviewed for possible security issues
related to invalid combinations of the fields.
[...]
> > [...]
> >
> >> @@ -330,10 +790,286 @@
> >> }
> >>
> >> /**
> >> - * Parse Spectral Band Replication extension data; reference: table 4.55.
> >> + * Dequantize and scale spectral data; reference: 4.6.3.3.
> >> *
> >> + * @param icoef array of quantized spectral data
> >> + * @param band_type array of the used band type
> >> + * @param sf array of scalefactors or intensity stereo positions
> >> + * @param coef array of dequantized, scaled spectral data
> >> + */
> >> +static void dequant(AACContext * ac, float coef[1024], const int icoef[1024], float sf[120],
> >> + const IndividualChannelStream * ics, enum BandType band_type[120]) {
> >> + const uint16_t * offsets = ics->swb_offset;
> >> + const int c = 1024/ics->num_window_groups;
> >> + int g, i, group, k, idx = 0;
> >> +
> >> + for (g = 0; g < ics->num_window_groups; g++) {
> >> + memset(coef + g * 128 + offsets[ics->max_sfb], 0, sizeof(float)*(c - offsets[ics->max_sfb]));
> >> + for (i = 0; i < ics->max_sfb; i++, idx++) {
> >> + if (band_type[idx] == NOISE_BT) {
> >> + const float scale = sf[idx] / ((offsets[i+1] - offsets[i]) * PNS_MEAN_ENERGY);
> >> + for (group = 0; group < ics->group_len[g]; group++) {
> >> + for (k = offsets[i]; k < offsets[i+1]; k++)
> >> + coef[group*128+k] = lcg_random(&ac->random_state) * scale;
> >> + }
> >> + } else if (band_type[idx] != INTENSITY_BT && band_type[idx] != INTENSITY_BT2) {
> >> + for (group = 0; group < ics->group_len[g]; group++) {
> >> + for (k = offsets[i]; k < offsets[i+1]; k++) {
> >> + coef[group*128+k] = ivquant(icoef[group*128+k]) * sf[idx];
> >> + }
> >> + }
> >> + }
> >> + }
> >> + coef += ics->group_len[g]*128;
> >> + icoef += ics->group_len[g]*128;
> >> + }
> >> +}
> >
> > it might be better to add 128 just outside the innermost loop to avoid
> > the "group*128 +" inside
>
> They would need to be reset the end of each iteration over i. I moved
> the group loops inside the conditions recently as I recall. What do
> you suggest? Add 128 to coef and icoef through each iteration over
> group and then reset?
I suggest you benchmark it, if it makes no speed difference they can stay
as they are.
[...]
> >> + }
> >> + domain = get_bits1(gb);
> >> +
> >> + if (is_indep_coup) {
> >> + coup->coupling_point = AFTER_IMDCT;
> >> + } else if(domain) {
> >> + coup->coupling_point = BETWEEN_TNS_AND_IMDCT;
> >> + } else
> >> + coup->coupling_point = BEFORE_TNS;
> >
> > :/ i thought the bits were at least stored together, what morons designed
> > this!?
> >
> > coup->coupling_point= get_bits1(gb);
> > ...
> > coup->coupling_point+= 2*get_bits1(gb);
> >
> > still is a possibility though that is cleaner
> > btw, what is the 4th possibility for?
>
> is_indep_coup = 1 overrides whatever domain is so the 4th case is
> redundant. Though theoretically, domain must be 1 if is_indep_coup is
> 1. is_indep_coup = 1 and domain = 0 is theoretically invalid.
>
> coup->coupling_point = 2*get_bits1(gb);
> ...
> coup->coupling_point += get_bits1(gb) && !coup->coupling_point;
> ?
hmm, this seems unneccesarily complex
and the invalid value should be checked for with av_log&return -1
[...]
> >> +
> >> +static int aac_decode_frame(AVCodecContext * avccontext, void * data, int * data_size, const uint8_t * buf, int buf_size) {
> >> + AACContext * ac = avccontext->priv_data;
> >> + GetBitContext gb;
> >> + enum RawDataBlockType elem_type;
> >> + int err, elem_id;
> >> +
> >> + init_get_bits(&gb, buf, buf_size*8);
> >> +
> >> + // parse
> >> + while ((elem_type = get_bits(&gb, 3)) != TYPE_END) {
> >> + elem_id = get_bits(&gb, 4);
> >> + err = -1;
> >> + switch (elem_type) {
> >> +
> >> + case TYPE_SCE:
> >> + if(!ac->che[TYPE_SCE][elem_id]) {
> >> + if(elem_id == 1 && ac->che[TYPE_LFE][0]) {
> >> + /* Some streams incorrectly code 5.1 audio as SCE[0] CPE[0] CPE[1] SCE[1]
> >> + instead of SCE[0] CPE[0] CPE[0] LFE[0].
> >> + If we seem to have encountered such a stream,
> >> + transfer the LFE[0] element to SCE[1] */
> >> + ac->che[TYPE_SCE][elem_id] = ac->che[TYPE_LFE][0];
> >> + ac->che[TYPE_LFE][0] = NULL;
> >> + } else
> >> + break;
> >> + }
> >> + err = decode_ics(ac, &ac->che[TYPE_SCE][elem_id]->ch[0], &gb, 0, 0);
> >> + break;
> >> +
> >> + case TYPE_CPE:
> >> + if (ac->che[TYPE_CPE][elem_id])
> >> + err = decode_cpe(ac, &gb, elem_id);
> >> + break;
> >> +
> >> + case TYPE_FIL:
> >> + if (elem_id == 15)
> >> + elem_id += get_bits(&gb, 8) - 1;
> >> + while (elem_id > 0)
> >> + elem_id -= decode_extension_payload(ac, &gb, elem_id);
> >> + err = 0; /* FIXME */
> >> + break;
> >> +
> >> + case TYPE_PCE:
> >> + {
> >> + enum ChannelPosition new_che_pos[4][MAX_ELEM_ID];
> >> + memset(new_che_pos, 0, 4 * MAX_ELEM_ID * sizeof(new_che_pos[0][0]));
> >> + if((err = decode_pce(ac, new_che_pos, &gb)))
> >> + break;
> >> + err = output_configure(ac, ac->che_pos, new_che_pos);
> >> + break;
> >> + }
> >> +
> >> + case TYPE_DSE:
> >> + skip_data_stream_element(&gb);
> >> + err = 0;
> >> + break;
> >> +
> >> + case TYPE_CCE:
> >> + if (ac->che[TYPE_CCE][elem_id])
> >> + err = decode_cce(ac, &gb, elem_id);
> >> + break;
> >> +
> >> + case TYPE_LFE:
> >> + if (ac->che[TYPE_LFE][elem_id])
> >> + err = decode_ics(ac, &ac->che[TYPE_LFE][elem_id]->ch[0], &gb, 0, 0);
> >> + break;
> >
> > these checks could be factorized out
> > if(elem_type < C && !ac->che[elem_type][elem_id])
> > return -1;
>
> The TYPE_SCE case has something slightly different so it will have to be:
>
> if(elem_type && elem_type < TYPE_DSE && !ac->che[elem_type][elem_id])
> return -1;
TYPE_SCE can be dealt with before the error check.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20080810/5647e5bc/attachment.pgp>
More information about the ffmpeg-devel
mailing list