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

[Claudio Freire]

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).