[FFmpeg-devel] [PATCH 2/3] libavcodec: ac3 floating point decoder fixes

[Babic, Nedeljko]

All the channels are used, they are just mixed in downmix.

Handling of blocks with different length in downmix is erroneous in current code and can cause incorrect
decoding and our patch is fixing this.

I can provide graphs for millers_crossing_4.0_mono.pcm generated before and after patch is applied where
this can be clearly seen if needed.

-Nedeljko
________________________________________
From: Michael Niedermayer [michaelni at gmx.at]
Sent: Tuesday, November 27, 2012 23:45
To: FFmpeg development discussions and patches
Cc: Babic, Nedeljko; Lukac, Zeljko
Subject: Re: [FFmpeg-devel] [PATCH 2/3] libavcodec: ac3 floating point decoder fixes

On Tue, Nov 27, 2012 at 05:27:56PM +0100, Nedeljko Babic wrote:
> Glitch caused by upmix fixed by changing the order
>  of imdct and downmix and hence eliminating upmix.
> Fate tests changed and new referent pcms generated
>  to support these changes.
>
> Signed-off-by: Nedeljko Babic <nbabic at mips.com>
> ---
>  libavcodec/ac3dec.c |   60 +++-----------------------------------------------
>  tests/fate/ac3.mak  |    8 +++---
>  2 files changed, 8 insertions(+), 60 deletions(-)
>
> diff --git a/libavcodec/ac3dec.c b/libavcodec/ac3dec.c
> index 7fb380c..0fe6673 100644
> --- a/libavcodec/ac3dec.c
> +++ b/libavcodec/ac3dec.c
> @@ -187,7 +187,6 @@ static av_cold int ac3_decode_init(AVCodecContext *avctx)
>              avctx->request_channels <= 2) {
>          avctx->channels = avctx->request_channels;
>      }
> -    s->downmixed = 1;
>
>      avcodec_get_frame_defaults(&s->frame);
>      avctx->coded_frame = &s->frame;
> @@ -621,34 +620,6 @@ static inline void do_imdct(AC3DecodeContext *s, int channels)
>  }
>
>  /**
> - * Upmix delay samples from stereo to original channel layout.
> - */
> -static void ac3_upmix_delay(AC3DecodeContext *s)
> -{
> -    int channel_data_size = sizeof(s->delay[0]);
> -    switch (s->channel_mode) {
> -    case AC3_CHMODE_DUALMONO:
> -    case AC3_CHMODE_STEREO:
> -        /* upmix mono to stereo */
> -        memcpy(s->delay[1], s->delay[0], channel_data_size);
> -        break;
> -    case AC3_CHMODE_2F2R:
> -        memset(s->delay[3], 0, channel_data_size);
> -    case AC3_CHMODE_2F1R:
> -        memset(s->delay[2], 0, channel_data_size);
> -        break;
> -    case AC3_CHMODE_3F2R:
> -        memset(s->delay[4], 0, channel_data_size);
> -    case AC3_CHMODE_3F1R:
> -        memset(s->delay[3], 0, channel_data_size);
> -    case AC3_CHMODE_3F:
> -        memcpy(s->delay[2], s->delay[1], channel_data_size);
> -        memset(s->delay[1], 0, channel_data_size);
> -        break;
> -    }
> -}
> -
> -/**
>   * Decode band structure for coupling, spectral extension, or enhanced coupling.
>   * The band structure defines how many subbands are in each band.  For each
>   * subband in the range, 1 means it is combined with the previous band, and 0
> @@ -1221,40 +1192,17 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>          ff_eac3_apply_spectral_extension(s);
>      }
>
> +    do_imdct(s, s->channels);
> +
>      /* downmix and MDCT. order depends on whether block switching is used for
>         any channel in this block. this is because coefficients for the long
>         and short transforms cannot be mixed. */
>      downmix_output = s->channels != s->out_channels &&
>                       !((s->output_mode & AC3_OUTPUT_LFEON) &&
>                       s->fbw_channels == s->out_channels);
> -    if (different_transforms) {
> -        /* the delay samples have already been downmixed, so we upmix the delay
> -           samples in order to reconstruct all channels before downmixing. */
> -        if (s->downmixed) {
> -            s->downmixed = 0;
> -            ac3_upmix_delay(s);
> -        }
>
> -        do_imdct(s, s->channels);
> -
> -        if (downmix_output) {
> -            s->ac3dsp.downmix(s->output, s->downmix_coeffs,
> -                              s->out_channels, s->fbw_channels, 256);
> -        }
> -    } else {
> -        if (downmix_output) {
> -            s->ac3dsp.downmix(s->transform_coeffs + 1, s->downmix_coeffs,
> -                              s->out_channels, s->fbw_channels, 256);
> -        }
> -
> -        if (downmix_output && !s->downmixed) {
> -            s->downmixed = 1;
> -            s->ac3dsp.downmix(s->delay, s->downmix_coeffs, s->out_channels,
> -                              s->fbw_channels, 128);
> -        }
> -
> -        do_imdct(s, s->out_channels);

this change would cause the decoder to unneccessarily imdct transform
channels that are subsequently not used

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

What does censorship reveal? It reveals fear. -- Julian Assange