[FFmpeg-devel] [PATCH] AAC decoder round 8

[Michael Niedermayer]

On Fri, Aug 15, 2008 at 04:12:48PM +0100, Robert Swain wrote:
[...]
> >> +    const int is8 = ics->window_sequence[0] == EIGHT_SHORT_SEQUENCE;
> >> +    const int tns_max_order = is8 ? 7 : ac->m4ac.object_type == AOT_AAC_MAIN ? 20 : 12;
> >> +    for (w = 0; w < ics->num_windows; w++) {
> >> +        tns->n_filt[w] = get_bits(gb, 2 - is8);
> >> +
> >> +        if (tns->n_filt[w])
> >> +            coef_res = get_bits1(gb) + 3;
> >> +
> >> +        for (filt = 0; filt < tns->n_filt[w]; filt++) {
> >> +            tns->length[w][filt] = get_bits(gb, 6 - 2*is8);
> >> +
> >
> >> +            if ((tns->order[w][filt] = get_bits(gb, 5 - 2*is8)) <= tns_max_order) {
> >> +                tns->direction[w][filt] = get_bits1(gb);
> >> +                coef_compress = get_bits1(gb);
> >> +                coef_len = coef_res - coef_compress;
> >> +                tns->tmp2_map[w][filt] = tns_tmp2_map[2*coef_compress + coef_res - 3];
> >
> > the 3 can be moved to "coef_len = coef_res - coef_compress + 3"
> 
> But, unless I'm missing something, that will change the behaviour of
> the code as more bits will be read in the loop you quoted just below.

hmmm, removing the + 3 from "coef_res = get_bits1(gb) + 3;" and adding 3
to all uses of coef_res should be fine.


> 
> >> +
> >> +                for (i = 0; i < tns->order[w][filt]; i++)
> >> +                    tns->coef[w][filt][i] = get_bits(gb, coef_len);
> >
> > tns->coef is only used to index into tmp2_map thus
> > tns->coef could already contain the values from tmp2_map
> > this also would make the tmp2_map field unneeded in the struct
> 
> OK, I'll look into this.
> 
> >> +            } else {
> >> +                av_log(ac->avccontext, AV_LOG_ERROR, "TNS filter order %d is greater than maximum %d.",
> >> +                       tns->order[w][filt], tns_max_order);
> >> +                tns->order[w][filt] = 0;
> >> +                return -1;
> >> +            }
> >
> > if(... > tns_max_order){
> >    ...
> >    return -1
> > }
> > ...
> >
> > seems cleaner to me
> 
> Done.
> 
> >> +        }
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +/**
> >>   * Decode Mid/Side data; reference: table 4.54.
> >>   *
> >>   * @param   ms_present  Indicates mid/side stereo presence. [0] mask is all 0s;
> >
> >> @@ -1067,6 +1116,71 @@
> >>  }
> >>
> >>  /**
> >> + * Decode Temporal Noise Shaping filter coefficients and apply all-pole filters; reference: 4.6.9.3.
> >> + *
> >> + * @param   decode  1 if tool is used normally, 0 if tool is used in LTP.
> >> + * @param   coef    spectral coefficients
> >> + */
> >> +static void apply_tns(float coef[1024], TemporalNoiseShaping * tns, IndividualChannelStream * ics, int decode) {
> >> +    const int mmm = FFMIN(ics->tns_max_bands,  ics->max_sfb);
> >> +    int w, filt, m, i, ib;
> >> +    int bottom, top, order, start, end, size, inc;
> >> +    float tmp;
> >> +    float lpc[TNS_MAX_ORDER + 1], b[2 * TNS_MAX_ORDER];
> >> +
> >> +    for (w = 0; w < ics->num_windows; w++) {
> >> +        bottom = ics->num_swb;
> >> +        for (filt = 0; filt < tns->n_filt[w]; filt++) {
> >> +            top = bottom;
> >> +            bottom = FFMAX(                  0, top - tns->length[w][filt]);
> >
> >> +            order  = FFMIN(tns->order[w][filt], TNS_MAX_ORDER);
> >
> > useless?
> 
> Indeed. Removed. Should the 'order' variable be removed as well or is
> accessing it faster than accessing order[][]?

dunno, if its used in the inner loop then order is probably usefull


[...]
> > b is not truly needed, coef[] can be used i its place
> > also this is likely relevant to overal codec speed so it
> > should be written more with speed than compactness in mind
> 
> Does that mean you want me to rewrite it?

of course :)


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20080815/0c379a8a/attachment.pgp>