[FFmpeg-devel] AAC decoder round 6
Michael Niedermayer
michaelni
Sun Aug 10 22:14:24 CEST 2008
On Sun, Aug 10, 2008 at 08:27:47PM +0100, Robert Swain wrote:
> 2008/8/10 Michael Niedermayer <michaelni at gmx.at>:
> > 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
>
> A note saying that they're going to be used by other files? Isn't that
> obvious from them being in the file containing shared declarations?
no, globals that are used by just one file are a reason to reject a patch
if i dont i have missed the fact.
with no note i have to remember which tables kostya wants to use, so i know
what is ok because a future patch will need it and what is not ok ...
> None of the other tables in aactab.h have any notes saying that
> they're shared.
>
> > [...]
> >>
> >> > [...]
> >> >> +
> >> >> +/**
> >> >> + * 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
>
> Hmm, OK. I didn't know about this. It can be changed as soon as it is
> committed. There are other unsupported features that can also use
> this.
just commit it and use it
>
> > [...]
> >> >> + 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.
>
> Hmm. I'm tempted to memset ics to 0 in the error cases.
iam not against that ...
>
> >> > [...]
> >> >
> >> >> @@ -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.
>
> OK. On a Core Duo 1.83GHz, 5 repetitions interleaved...
>
> Unpatched:
> real 0m8.697s
> user 0m8.052s
> sys 0m0.494s
and START/STOP_TIMER over the function?
[...]
> > [...]
> >> >> + }
> >> >> + 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
>
> It's not invalid in that it should be checked and cause an error. It's
> just that the value of domain is ignored when is_indep_coup is set. I
Where does the spec say this?
If the spec says that one bit switches between doing it before and after A
and says the second switches before and after B then the order of A and B
implicate which combination is invalid.
> said it was theoretically invalid to have is_indep_coup = 1 and domain
> = 0 as is_indep_coup => coupling occurs after imdct => domain = 1. But
> in practice, domain is ignored when is_indep_coup = 1.
>
> So, it's just whether you prefer the if() {} else if() {} else {} or
> what I suggested above. Or something else.
>
> > [...]
> >> >> +
> >> >> +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.
>
> Do you just mean:
>
> if(elem_type == TYPE_SCE && !ac->che[TYPE_SCE][elem_id] &&
> elem_id = 1 && ac->che[TYPE_LFE][0]) {
> ac->che[TYPE_SCE][elem_id] = ac->che[TYPE_LFE][0];
> ac->che[TYPE_LFE][0] = NULL;
> }
> if(elem_type < TYPE_DSE && !ac->che[elem_type][elem_id])
> return -1;
yes
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/ca2d1d80/attachment.pgp>
More information about the ffmpeg-devel
mailing list