[FFmpeg-devel] [PATCH 2/6] aacenc: Improve Intensity Stereo phase detection

[Rostislav Pehlivanov]

>L34/R34
Calculating the 3/4 power of the coefficients isn't really that expensive
to do twice, so I'll just scrap the whole idea.

>identation
How'd I miss that? Fixed.

>phase
Copied that from an older revision, missed that '*' sign and didn't notice,
fixed.

Will wait a while to see whether the other commits look good before sending
the fixed one.

On 4 August 2015 at 08:31, Claudio Freire <klaussfreire at gmail.com> wrote:

> On Wed, Jul 29, 2015 at 1:44 AM, Rostislav Pehlivanov
> <atomnuker at gmail.com> wrote:
> > +    if (cpe->ms_mode)
> > +        phase = 1 - 2 * cpe->ms_mask[w*16+g];
>
>
> Shouldn't it be ?
>
> phase *= 1 - ... ;
>
> phase is an argument, the original code would step on it, with a value
> that doesn't depend on phase, so it would fail to evaluate both
> phases. Using phase *= would make sure to test both phases.
>
> Well, that's the general idea, except it breaks the phase assigned to
> the struct. Something like the following does work though:
>
>     ephase = phase;
>     if (cpe->ms_mode)
>         ephase *= 1 - 2 * cpe->ms_mask[w*16+g];
>
> and then change all uses of phase into ephase, except the last that
> remains:
>
>     is_error.phase = phase; // original phase
>     is_error.pass  = dist2 <= dist1;
>
>
> On Wed, Jul 29, 2015 at 1:44 AM, Rostislav Pehlivanov
> <atomnuker at gmail.com> wrote:
> >      for (w = 0; w < 128; w++)
> >          if (sce1->band_type[w] >= INTENSITY_BT2)
> >              sce1->band_type[w] = 0;
> >
> > -    if (!cpe->common_window)
> > -        return;
> > -    for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w])
> {
> > -        start = 0;
> > -        for (g = 0;  g < sce0->ics.num_swb; g++) {
> > -            if (start*freq_mult > INT_STEREO_LOW_LIMIT*(lambda/170.0f)
> &&
> > -                cpe->ch[0].band_type[w*16+g] != NOISE_BT &&
> !cpe->ch[0].zeroes[w*16+g] &&
> > -                cpe->ch[1].band_type[w*16+g] != NOISE_BT &&
> !cpe->ch[1].zeroes[w*16+g]) {
> > -                int phase = 0;
> > -                float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
> > -                float dist1 = 0.0f, dist2 = 0.0f;
> > +        if (!cpe->common_window)
> > +            return;
> > +        for (w = 0; w < sce0->ics.num_windows; w +=
> sce0->ics.group_len[w]) {
> > +            start = 0;
> > +            for (g = 0;  g < sce0->ics.num_swb; g++) {
> > +                if (start*freq_mult >
> INT_STEREO_LOW_LIMIT*(s->lambda/170.0f) &&
>
> This looks strange. As it is that patch, it ends up with code like:
>
> >    for (w = 0; w < 128; w++)
> >        if (sce1->band_type[w] >= INTENSITY_BT2)
> >            sce1->band_type[w] = 0;
> >
> >        if (!cpe->common_window)
> >            return;
> >        for (w = 0; w < sce0->ics.num_windows; w +=
> sce0->ics.group_len[w]) {
> >            start = 0;
> >            for (g = 0;  g < sce0->ics.num_swb; g++) {
>
> Which looks wrong. Bad indentation right?
>
> I think you meant:
>
>     for (w = 0; w < 128; w++)
>         if (sce1->band_type[w] >= INTENSITY_BT2)
>             sce1->band_type[w] = 0;
>
>     if (!cpe->common_window)
>         return;
>     for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) {
>         start = 0;
>         for (g = 0;  g < sce0->ics.num_swb; g++) {
>
> A big part of the diff in that hunk is reindent, so I believe if you
> fix that indentation snafu the patch will shrink.
>
> On Wed, Jul 29, 2015 at 1:44 AM, Rostislav Pehlivanov
> <atomnuker at gmail.com> wrote:
> >                  for (w2 = 0; w2 < sce0->ics.group_len[w]; w2++) {
> > +                    abs_pow34_v(L34, sce0->coeffs+start+(w+w2)*128,
> sce0->ics.swb_sizes[g]);
> > +                    abs_pow34_v(R34, sce1->coeffs+start+(w+w2)*128,
> sce0->ics.swb_sizes[g]);
> >                      for (i = 0; i < sce0->ics.swb_sizes[g]; i++) {
> >                          float coef0 = sce0->pcoeffs[start+(w+w2)*128+i];
> >                          float coef1 = sce1->pcoeffs[start+(w+w2)*128+i];
> > -                        phase += coef0*coef1 >= 0.0f ? 1 : -1;
> >                          ener0 += coef0*coef0;
> >                          ener1 += coef1*coef1;
> >                          ener01 += (coef0 + coef1)*(coef0 + coef1);
> >                      }
> >                  }
>
> Careful, you're stepping on L34 and R34 on eight_short_window, and
> passing the last results only to calc_encoding_err_is.
>
> In fact, I'm thinking I may have induced you to make that mistake when
> I suggested not to compute R34 / L34 twice (once for each phase),
> since L34 and R34 only have room for one window, and
> calc_encoding_err_is needs to process a whole window group.
>
> I think you'll have to move it back to inside calc_encoing_err_is and
> just compute it twice. Redundant, but at least it's correct.
>
> Also, you should use pcoeffs (coeffs will have M/S applied to it when
> ms_mask).
>