[FFmpeg-devel] AAC decoder round 5
Michael Niedermayer
michaelni
Fri Aug 8 01:44:24 CEST 2008
On Thu, Aug 07, 2008 at 11:53:39PM +0100, Robert Swain wrote:
> 2008/8/7 Michael Niedermayer <michaelni at gmx.at>:
[...]
> > [...]
> >> > [...]
> >> >> +
> >> >> +/**
> >> >> + * channel coupling transformation interface
> >> >> + *
> >> >> + * @param index index into coupling gain array
> >> >> + * @param apply_coupling_method pointer to (in)dependent coupling function
> >> >> + */
> >> >> +static void apply_channel_coupling(AACContext * ac, ChannelElement * cc,
> >> >> + void (*apply_coupling_method)(AACContext * ac, SingleChannelElement * sce, ChannelElement * cc, int index))
> >> >> +{
> >> >> + int c;
> >> >> + int index = 0;
> >> >> + ChannelCoupling * coup = &cc->coup;
> >> >> + for (c = 0; c <= coup->num_coupled; c++) {
> >> >> + if ( !coup->is_cpe[c] && ac->che[ID_SCE][coup->tag_select[c]]) {
> >> >> + apply_coupling_method(ac, &ac->che[ID_SCE][coup->tag_select[c]]->ch[0], cc, index++);
> >> >> + } else if(coup->is_cpe[c] && ac->che[ID_CPE][coup->tag_select[c]]) {
> >> >
> >> >> + if (!coup->l[c] && !coup->r[c]) {
> >> >> + apply_coupling_method(ac, &ac->che[ID_CPE][coup->tag_select[c]]->ch[0], cc, index);
> >> >> + apply_coupling_method(ac, &ac->che[ID_CPE][coup->tag_select[c]]->ch[1], cc, index++);
> >> >> + }
> >> >
> >> > The struct docs say r/l are "apply gain to r/l channel" this contradicts the
> >> > 0/0 case. which is correct and which is wrong?
> >>
> >> In the spec, for cc_l[] it says:
> >>
> >> "one bit indicating that a list of gain_element values is applied to the left
> >> channel of a channel pair."
> >>
> >> The code that uses it implies that description isn't strictly true. In
> >> the !l[] && !r[] case, there is a shared list of gain_element values.
> >> So l[] and r[] indicate the presence of a channel-specific list of
> >> gain_element values. I'll change the comments.
> >
> > if(coup->ch_select!=2){
> > apply_coupling_method(ac, &ac->che[ID_CPE][coup->tag_select[c]]->ch[0], cc, index);
> > if(coup->ch_select!=0)
> > index++;
> > }
> > if(coup->ch_select!=1)
> > apply_coupling_method(ac, &ac->che[ID_CPE][coup->tag_select[c]]->ch[1], cc, index++);
>
> I don't know if it was what was intended but having ch_select[c] =
i was thinking of a ch_select[c]= get_bits(2) yes ...
> l[c] + 2*r[c]; would make this work. Then ch_select[c] == 0 for both
> 0, 1 for l[c] is 1, 2 for r[c] is 1, and 3 for both 1. It saves 1 line
> and 2 calls to apply_coupling_method(). Applied.
>
> >> > [...]
> >> >> +
> >> >> +/**
> >> >> + * 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 mixdown_and_convert_to_int16(AVCodecContext * avccontext, uint16_t * data, int * data_size) {
> >> >> + AACContext * ac = avccontext->priv_data;
> >> >> + int i;
> >> >> + float *c, *l, *r, *sl, *sr, *out;
> >> >> +
> >> >> + if (!ac->is_saved) {
> >> >> + ac->is_saved = 1;
> >> >> + *data_size = 0;
> >> >> + return 0;
> >> >> + }
> >> >> +
> >> >> + i = 1024 * avccontext->channels * sizeof(int16_t);
> >> >> + if(*data_size < i) {
> >> >> + av_log(avccontext, AV_LOG_ERROR,
> >> >> + "Output buffer too small (%d) or trying to output too many samples (%d) for this frame.\n",
> >> >> + *data_size, i);
> >> >> + return -1;
> >> >> + }
> >> >> + *data_size = i;
> >> >> +
> >> >> + if(ac->mm[MIXDOWN_CENTER]) {
> >> >> + /* matrix mix-down */
> >> >> + l = ac->mm[MIXDOWN_FRONT ]->ch[0].ret;
> >> >> + r = ac->mm[MIXDOWN_FRONT ]->ch[1].ret;
> >> >> + c = ac->mm[MIXDOWN_CENTER]->ch[0].ret;
> >> >> + sl = ac->mm[MIXDOWN_BACK ]->ch[0].ret;
> >> >> + sr = ac->mm[MIXDOWN_BACK ]->ch[1].ret;
> >> >> + out = ac->interleaved_output;
> >> >> +
> >> >> + // XXX dsputil-ize
> >> >> + if(avccontext->channels == 2) {
> >> >> + if(ac->pcs.pseudo_surround) {
> >> >> + for(i = 0; i < 1024; i++) {
> >> >> + *out++ = *l++ + *c - *sl - *sr + ac->add_bias;
> >> >> + *out++ = *r++ + *c++ + *sl++ + *sr++ - ac->add_bias * 3;
> >> >> + }
> >> >> + } else {
> >> >> + for(i = 0; i < 1024; i++) {
> >> >> + *out++ = *l++ + *c + *sl++ - ac->add_bias * 2;
> >> >> + *out++ = *r++ + *c++ + *sr++ - ac->add_bias * 2;
> >> >> + }
> >> >> + }
> >> >> +
> >> >> + } else {
> >> >> + assert(avccontext->channels == 1);
> >> >> + for(i = 0; i < 1024; i++) {
> >> >> + *out++ = *l++ + *r++ + *c++ + *sl++ + *sr++ - ac->add_bias * 4;
> >> >> + }
> >> >> + }
> >> >> +
> >> >> + ac->dsp.float_to_int16(data, ac->interleaved_output, 1024 * avccontext->channels);
> >> >> + } else {
> >> >> + ac->dsp.float_to_int16_interleave(data, (const float **)ac->output_data, 1024, avccontext->channels);
> >> >> + }
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >
> >> > mixdown should be done prior to the IMDCT when possible and the IMDCT skipped
> >> > for channels that are not needed, or _ALL_ mixdown code should be removed
> >> > from the AAC decoder, as in that case mixdown can be done outside of the
> >> > decoder equally well and cleaner.
> >>
> >> I think doing pre-IMDCT mix-down will be complicated because of the
> >> different windows and their overlaps. As a consequence of these issues
> >> the optimisation may or may not be worth it. I think the overlapping
> >> simplifications I had planned when transitioning to using imdct_half()
> >> may help so I should probably do those first.
> >>
> >> *** In the spec they advise against using the matrix mix-down method
> >> so I think all this matrix mix-down code should be dropped in favour
> >> of generic channel mixing either pre-IMDCT or post decoding, unless
> >> someone knows of a good reason why it should be kept. I think Andreas
> >> implemented this code but I'm not sure.
> >
> > also droping it will make it hit svn sooner ...
>
> I'll remove it from SoC. Patch attached
> (20080807-2351-remove_mixdown.diff). After this I'll reindent, remove
> ProgramConfig, change the code accordingly, rename variables and
> functions accordingly and I also spotted that che_type[][] should be
> renamed to che_pos[][] or something similar as the enum was changed
> from ChannelType to ChannelPosition.
ok
>
> >> >> +
> >> >> +
> >> >> +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 RawDataBlockID id;
> >> >> + int err, tag;
> >> >> +
> >> >> + init_get_bits(&gb, buf, buf_size*8);
> >> >> +
> >> >> + // parse
> >> >> + while ((id = get_bits(&gb, 3)) != ID_END) {
> >> >> + tag = get_bits(&gb, 4);
> >> >> + err = -1;
> >> >> + switch (id) {
> >> >> +
> >> >
> >> >> + case ID_SCE:
> >> >> + if(!ac->che[ID_SCE][tag]) {
> >> >
> >> > this id / tag naming confuses the hell out of me ...
> >> > Both values together identify the "thing". the id here is the
> >> > type (channel pair / single channel / ...)
> >>
> >> Why is it confusing? As you say, the id is the type and the tag is
> >> (and I know you don't like this but I'm going to use it anyway because
> >> I'm happy with it) the instance of that type.
> >
> > Well iam not a native englishman ...
> > but to me id is something by which a specific individual or instance can
> > uniquely be identified. This isnt true here ...
> >
> > That is, the number on the license plate of a car is an ID,
> > Rolls Royce Phantom II is not an ID its the type or model.
>
> Hmm, I agree. In the spec it uses ID but something like element_type
> and element_id instead of id and tag respectively would be better,
> wouldn't you agree?
yes
> I suppose identifier is a better word than
> instance in this case, I've just not thought of using it as the other
> variable was id.
>
> >> > [...]
> >> >> Index: libavcodec/aac.h
> >> >> ===================================================================
> >> >> --- libavcodec/aac.h (revision 14624)
> >> >> +++ libavcodec/aac.h (working copy)
> >> >> @@ -42,8 +44,63 @@
> >> >> ff_aac_spectral_codes[num], sizeof(ff_aac_spectral_codes[num][0]), sizeof(ff_aac_spectral_codes[num][0]), \
> >> >> size);
> >> >>
> >> >> +#define MAX_CHANNELS 64
> >> >
> >> > ok as long as its not causing huge arrays, like if there was a use like
> >> > array[MAX_FRAME][MAX_TAGS][MAX_BANDS][MAX_CHANNELS] ...
> >>
> >> Please define huge.
> >
> > huge being 10k for the context IMHO, that is if you can safe 10k by
> > making MAX_CHANNELS a realistic number please do.
> > if it just safes 500 bytes its not worth it if it makes the code more
> > complex ...
>
> OK, I'll have a check. Are you mainly concerned about the main
> AACContext rather than the individual channel elements? Of course it's
> good to minimise them all as long as they don't cause obfuscated code
> (though it shouldn't hurt to reduce MAX_CHANNELS I wouldn't have
> thought).
Iam concerned about all large arrays that are partly unused ...
[...]
> > [...]
> >> >> + int num_coupled; ///< number of target elements
> >> >
> >> >> + int is_cpe[9]; ///< Set if target is an CPE (otherwise it's an SCE).
> >> >
> >> > Maybe some code could be simplified if these where ID_CPE/ID_SCE ?
> >>
> >> Do you mean if the array values were ID_CPE/ID_SCE?
> >
> > yes
> >
> >> I don't think it
> >> would allow any simplifications.
> >
> > hmm
>
> It could be simplified if the value of ch_select[] were 'fudged' to
> make it work (increment the index) for the ID_SCE case which is
> equivalent to the l[c] = 1, r[c] = 0 case so ch_select[c] would have
> to be set to 1 for it to work. Is that acceptable? If so the code in
> apply_channel_coupling() would look like the attached patch
> (20080807-2314-is_cpe_alteration.diff). diffstat says:
>
> aac.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
well, forget it this isnt important ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20080808/ace855c6/attachment.pgp>
More information about the ffmpeg-devel
mailing list