[FFmpeg-devel] [PATCH v2 3/8] aaccoder: fix M/S coding

[Claudio Freire]

On Thu, Jul 2, 2015 at 3:13 PM, Rostislav Pehlivanov
<atomnuker at gmail.com> wrote:
> There were some mistakes in the code for M/S stereo, this commit fixes them. The start variable was not being reset for every window and every access to the coefficients was incorrect as well. This fixes that by properly addressing the coefficients using both windows and setting the start on every window to zero.
> ---
>  libavcodec/aaccoder.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c
> index 3fcc8b4..33cbe7b 100644
> --- a/libavcodec/aaccoder.c
> +++ b/libavcodec/aaccoder.c
> @@ -1143,6 +1143,7 @@ static void search_for_ms(AACEncContext *s, ChannelElement *cpe,
>      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 (!cpe->ch[0].zeroes[w*16+g] && !cpe->ch[1].zeroes[w*16+g]) {
>                  float dist1 = 0.0f, dist2 = 0.0f;
> @@ -1152,22 +1153,22 @@ static void search_for_ms(AACEncContext *s, ChannelElement *cpe,
>                      float minthr = FFMIN(band0->threshold, band1->threshold);
>                      float maxthr = FFMAX(band0->threshold, band1->threshold);
>                      for (i = 0; i < sce0->ics.swb_sizes[g]; i++) {
> -                        M[i] = (sce0->pcoeffs[start+w2*128+i]
> -                              + sce1->pcoeffs[start+w2*128+i]) * 0.5;
> +                        M[i] = (sce0->pcoeffs[start+(w+w2)*128+i]
> +                              + sce1->pcoeffs[start+(w+w2)*128+i]) * 0.5;
>                          S[i] =  M[i]
> -                              - sce1->pcoeffs[start+w2*128+i];
> +                              - sce1->pcoeffs[start+(w+w2)*128+i];
>                      }
> -                    abs_pow34_v(L34, sce0->coeffs+start+w2*128, sce0->ics.swb_sizes[g]);
> -                    abs_pow34_v(R34, sce1->coeffs+start+w2*128, sce0->ics.swb_sizes[g]);
> +                    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]);
>                      abs_pow34_v(M34, M,                         sce0->ics.swb_sizes[g]);
>                      abs_pow34_v(S34, S,                         sce0->ics.swb_sizes[g]);
> -                    dist1 += quantize_band_cost(s, sce0->coeffs + start + w2*128,
> +                    dist1 += quantize_band_cost(s, sce0->coeffs + start + (w+w2)*128,
>                                                  L34,
>                                                  sce0->ics.swb_sizes[g],
>                                                  sce0->sf_idx[(w+w2)*16+g],
>                                                  sce0->band_type[(w+w2)*16+g],
>                                                  lambda / band0->threshold, INFINITY, NULL);
> -                    dist1 += quantize_band_cost(s, sce1->coeffs + start + w2*128,
> +                    dist1 += quantize_band_cost(s, sce1->coeffs + start + (w+w2)*128,
>                                                  R34,
>                                                  sce1->ics.swb_sizes[g],
>                                                  sce1->sf_idx[(w+w2)*16+g],


LGTM.

Though I should probably mention that this patch shouldn't make any
difference in the current state of search_for_ms.

Unless there's an error somewhere (which is common in my experience
with the aac encoder, so the proposed code is more robust in that
regard and should be pushed), the current code will result in the same
behavior exactly.

However, that's not always the case in other cases with the same
coding pattern is used, usually in search_for_X functions, where start
is used to compute the frequency that corresponds to a particular
coefficient. In those uses, if start isn't based at 0 for any given
window, the frequency will be wrongly computed.

So I'd push this patch if only to keep things consistent, and to avoid
similar errors in the future (I've corrected tons of instances of this
in #2686).