[PR] avcodec/ac3dec_fixed: fix dither at exponent 24 (PR #24015)

PR #24015 opened by AYOUB NABIL BOUBAGRAT (ayoubnabil)
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24015
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24015.patch

the fixed decoder discarded fractional coefficient data before the imdct,
causing low-level coefficients to be truncated at large exponents. 
dithered bap 0 coefficients at exponent 24 were consequently quantized with a negative bias.
keep ac-3 coefficients in q2, compensate during windowing, and round the affected dither symmetrically. 
keep the forced e-ac-3 path in q0.
add a synthetic fate test covering the corrected output.
tested with fate-ac3 on x86-64, x86-32, and ubsan.


>From 99340678be03292725894380a4e7881b6e4b7c2c Mon Sep 17 00:00:00 2001
From: Ayoub Nabil <[email protected]>
Date: Wed, 5 Aug 2026 02:45:10 +0200
Subject: [PATCH] avcodec/ac3dec_fixed: fix dither at exponent 24

the fixed decoder discarded fractional coefficient data before the imdct, causing low-level coefficients to be truncated at large exponents. dithered bap 0 coefficients at exponent 24 were consequently quantized with a negative bias.

keep ac-3 coefficients in q2, compensate during windowing, and round the affected dither symmetrically. keep the forced e-ac-3 path in q0.

add a synthetic fate test covering the corrected output.
---
 libavcodec/ac3dec.c             | 70 +++++++++++++++++++++++++++++----
 libavcodec/ac3dec_fixed.c       | 54 +++++++++++++++++++++++++
 tests/fate/ac3.mak              | 24 ++++++++++-
 tests/ref/fate/ac3-fixed-dexp24 |  6 +++
 4 files changed, 146 insertions(+), 8 deletions(-)
 create mode 100644 tests/ref/fate/ac3-fixed-dexp24

diff --git a/libavcodec/ac3dec.c b/libavcodec/ac3dec.c
index 6f7306ea59..99df08c319 100644
--- a/libavcodec/ac3dec.c
+++ b/libavcodec/ac3dec.c
@@ -390,6 +390,27 @@ typedef struct mant_groups {
     int b4;
 } mant_groups;
 
+static av_always_inline int dequantize_coeff(int mantissa, int exponent,
+                                             int coeff_bits)
+{
+#if USE_FIXED
+    return (mantissa * (1 << coeff_bits)) >> exponent;
+#else
+    (void)coeff_bits;
+    return mantissa >> exponent;
+#endif
+}
+
+#if USE_FIXED
+static av_always_inline int dequantize_dexp24_dither(int mantissa)
+{
+    int scaled = mantissa * (1 << AC3_FIXED_COEFF_BITS);
+    int round  = 1 << (AC3_FIXED_EXPONENT_MAX - 1);
+
+    return (scaled + round - (scaled < 0)) >> AC3_FIXED_EXPONENT_MAX;
+}
+#endif
+
 /**
  * Decode the transform coefficients for a particular channel
  * reference: Section 7.3 Quantization and Decoding of Mantissas
@@ -402,6 +423,11 @@ static void ac3_decode_transform_coeffs_ch(AC3DecodeContext *s, int ch_index, ma
     int8_t *exps   = s->dexps[ch_index];
     int32_t *coeffs = s->fixed_coeffs[ch_index];
     int dither     = (ch_index == CPL_CH) || s->dither_flag[ch_index];
+#if USE_FIXED
+    int coeff_bits = fixed_coeff_bits(s);
+#else
+    int coeff_bits = 0;
+#endif
     GetBitContext *gbc = &s->gbc;
     int freq;
 
@@ -411,10 +437,19 @@ static void ac3_decode_transform_coeffs_ch(AC3DecodeContext *s, int ch_index, ma
         switch (bap) {
         case 0:
             /* random noise with approximate range of -0.707 to 0.707 */
-            if (dither)
+            if (dither) {
                 mantissa = (((av_lfg_get(&s->dith_state)>>8)*181)>>8) - 5931008;
-            else
+#if USE_FIXED
+                /* At dexp 24 the dither is below half a Q0 step. Keep two
+                 * fractional bits so it is not truncated to -1 or 0. */
+                if (coeff_bits && exps[freq] == AC3_FIXED_EXPONENT_MAX) {
+                    coeffs[freq] = dequantize_dexp24_dither(mantissa);
+                    continue;
+                }
+#endif
+            } else {
                 mantissa = 0;
+            }
             break;
         case 1:
             if (m->b1) {
@@ -466,7 +501,7 @@ static void ac3_decode_transform_coeffs_ch(AC3DecodeContext *s, int ch_index, ma
             mantissa = (unsigned)get_sbits(gbc, ff_ac3_quantization_tab[bap]) << (24 - ff_ac3_quantization_tab[bap]);
             break;
         }
-        coeffs[freq] = mantissa >> exps[freq];
+        coeffs[freq] = dequantize_coeff(mantissa, exps[freq], coeff_bits);
     }
 }
 
@@ -500,7 +535,8 @@ static inline void decode_transform_coeffs_ch(AC3DecodeContext *s, int blk,
         if (CONFIG_EAC3_DECODER && !blk)
             ff_eac3_decode_transform_coeffs_aht_ch(s, ch);
         for (bin = s->start_freq[ch]; bin < s->end_freq[ch]; bin++) {
-            s->fixed_coeffs[ch][bin] = s->pre_mantissa[ch][bin][blk] >> s->dexps[ch][bin];
+            s->fixed_coeffs[ch][bin] = dequantize_coeff(
+                s->pre_mantissa[ch][bin][blk], s->dexps[ch][bin], 0);
         }
     }
 }
@@ -571,6 +607,9 @@ static void do_rematrixing(AC3DecodeContext *s)
 static inline void do_imdct(AC3DecodeContext *s, int channels, int offset)
 {
     int ch;
+#if USE_FIXED
+    int window_bits = 8 + fixed_coeff_bits(s);
+#endif
 
     for (ch = 1; ch <= channels; ch++) {
         if (s->block_switch[ch]) {
@@ -581,7 +620,7 @@ static inline void do_imdct(AC3DecodeContext *s, int channels, int offset)
             s->tx_fn_128(s->tx_128, s->tmp_output, x, sizeof(INTFLOAT));
 #if USE_FIXED
             s->fdsp->vector_fmul_window_scaled(s->outptr[ch - 1], s->delay[ch - 1 + offset],
-                                       s->tmp_output, s->window, 128, 8);
+                                       s->tmp_output, s->window, 128, window_bits);
 #else
             s->fdsp->vector_fmul_window(s->outptr[ch - 1], s->delay[ch - 1 + offset],
                                        s->tmp_output, s->window, 128);
@@ -593,7 +632,7 @@ static inline void do_imdct(AC3DecodeContext *s, int channels, int offset)
             s->tx_fn_256(s->tx_256, s->tmp_output, s->transform_coeffs[ch], sizeof(INTFLOAT));
 #if USE_FIXED
             s->fdsp->vector_fmul_window_scaled(s->outptr[ch - 1], s->delay[ch - 1 + offset],
-                                       s->tmp_output, s->window, 128, 8);
+                                       s->tmp_output, s->window, 128, window_bits);
 #else
             s->fdsp->vector_fmul_window(s->outptr[ch - 1], s->delay[ch - 1 + offset],
                                        s->tmp_output, s->window, 128);
@@ -1287,7 +1326,11 @@ static int decode_audio_block(AC3DecodeContext *s, int blk, int offset)
             gain = s->dynamic_range[audio_channel];
 
 #if USE_FIXED
-        scale_coefs(s->transform_coeffs[ch], s->fixed_coeffs[ch], gain, 256);
+        if (fixed_coeff_bits(s))
+            scale_coefs_q2(s->transform_coeffs[ch], s->fixed_coeffs[ch], gain,
+                           256);
+        else
+            scale_coefs(s->transform_coeffs[ch], s->fixed_coeffs[ch], gain, 256);
 #else
         if (s->target_level != 0)
           gain = gain * s->level_gain[audio_channel];
@@ -1356,6 +1399,9 @@ static int ac3_decode_frame(AVCodecContext *avctx, AVFrame *frame,
     AC3DecodeContext *s = avctx->priv_data;
     int blk, ch, err, offset, ret;
     int i;
+#if USE_FIXED
+    int previous_coeff_bits;
+#endif
     int skip = 0, got_independent_frame = 0;
     const uint8_t *channel_map;
     uint8_t extended_channel_map[EAC3_MAX_CHANNELS];
@@ -1390,6 +1436,9 @@ static int ac3_decode_frame(AVCodecContext *avctx, AVFrame *frame,
 
     buf = s->input_buffer;
 dependent_frame:
+#if USE_FIXED
+    previous_coeff_bits = fixed_coeff_bits(s);
+#endif
     /* initialize the GetBitContext with the start of valid AC-3 Frame */
     if ((ret = init_get_bits8(&s->gbc, buf, buf_size)) < 0)
         return ret;
@@ -1397,6 +1446,13 @@ dependent_frame:
     /* parse the syncinfo */
     err = parse_frame_header(s);
 
+#if USE_FIXED
+    /* Do not mix Q0 and Q2 overlap samples if a malformed or explicitly
+     * forced stream switches between E-AC-3 and AC-3. */
+    if (!err && previous_coeff_bits != fixed_coeff_bits(s))
+        memset(s->delay, 0, sizeof(s->delay));
+#endif
+
     if (err) {
         switch (err) {
         case AC3_PARSE_ERROR_SYNC:
diff --git a/libavcodec/ac3dec_fixed.c b/libavcodec/ac3dec_fixed.c
index 285553c22e..5fb07dc58f 100644
--- a/libavcodec/ac3dec_fixed.c
+++ b/libavcodec/ac3dec_fixed.c
@@ -55,6 +55,17 @@
 
 #include "ac3dec.h"
 
+/* Keep two fractional bits in fixed-point transform coefficients. */
+#define AC3_FIXED_COEFF_BITS 2
+#define AC3_FIXED_EXPONENT_MAX 24
+
+static av_always_inline int fixed_coeff_bits(const AC3DecodeContext *s)
+{
+    /* ac3_fixed normally decodes AC-3. Keep Q0 when it is explicitly forced
+     * to decode E-AC-3, whose AHT coefficient bounds are different. */
+    return s->eac3 ? 0 : AC3_FIXED_COEFF_BITS;
+}
+
 static const int end_freq_inv_tab[8] =
 {
     50529027, 44278013, 39403370, 32292987, 27356480, 23729101, 20951060, 18755316
@@ -124,6 +135,49 @@ static void scale_coefs (
     }
 }
 
+static void scale_coefs_q2(int32_t *dst, const int32_t *src, int dynrng,
+                           int len)
+{
+    int i, shift;
+    int mul;
+
+    mul = (dynrng & 0x1f) + 0x20;
+    shift = 4 - (sign_extend(dynrng, 9) >> 5);
+
+    /* AC-3 mantissas have magnitude at most 2^23, hence Q2 coefficients
+     * have magnitude at most 2^25. Coupling uses MULH(coeff * 2^4, coord)
+     * with coord < 2^31, so coupled coefficients are below 2^28.
+     * Rematrixing can at most double them, keeping src below 2^29. */
+    if (dynrng == 32) {
+        for (i = 0; i < len; i++)
+            dst[i] = src[i] * 4;
+        return;
+    }
+
+    if (shift >= 4) {
+        const int round = 1 << (shift - 1);
+        const int unit  = 1 << shift;
+
+        /* With shift >= 4, quotient * mul is below 2^25 * 63. Splitting
+         * quotient and remainder therefore keeps both products in int32_t. */
+        for (i = 0; i < len; i++) {
+            int quotient  = src[i] >> shift;
+            int remainder = src[i] - quotient * unit;
+
+            dst[i] = quotient * mul + ((remainder * mul + round) >> shift);
+        }
+    } else if (shift > 0) {
+        const int round = 1 << (shift - 1);
+
+        for (i = 0; i < len; i++)
+            dst[i] = av_clipl_int32(((int64_t)src[i] * mul + round) >> shift);
+    } else {
+        mul <<= -shift;
+        for (i = 0; i < len; i++)
+            dst[i] = av_clipl_int32((int64_t)src[i] * mul);
+    }
+}
+
 /**
  * Downmix samples from original signal to stereo or mono (this is for 16-bit samples
  * and fixed point decoder - original (for 32-bit samples) is in ac3dsp.c).
diff --git a/tests/fate/ac3.mak b/tests/fate/ac3.mak
index 875fa12418..f2497ec044 100644
--- a/tests/fate/ac3.mak
+++ b/tests/fate/ac3.mak
@@ -102,11 +102,33 @@ fate-ac3-fixed-encode-3: tests/data/asynth-44100-6.wav
 fate-ac3-fixed-encode-3: SRC = $(TARGET_PATH)/tests/data/asynth-44100-6.wav
 fate-ac3-fixed-encode-3: CMD = framecrc -i $(SRC) -c:a ac3_fixed -flags2 +fixed_frame_size -ab 256k -af aresample,atrim=start_sample=0:end_sample=12096
 
+# With coupling and rematrixing disabled, this produces bap=0, dexp=24 bins
+# whose dither affects the decoded output.
+tests/data/fate/ac3-fixed-dexp24.ac3: TAG = GEN
+tests/data/fate/ac3-fixed-dexp24.ac3: ffmpeg$(PROGSSUF)$(EXESUF) tests/data/asynth-44100-2.wav | tests/data/fate
+	$(M)$(TARGET_EXEC) $(TARGET_PATH)/ffmpeg$(PROGSSUF)$(EXESUF) \
+	-hide_banner -loglevel error -nostdin \
+	-i $(TARGET_PATH)/tests/data/asynth-44100-2.wav \
+	-c:a ac3_fixed -b:a 192k \
+	-channel_coupling 0 -stereo_rematrixing 0 -flags +bitexact \
+	-frames:a 173 -f ac3 -y $(TARGET_PATH)/$@
+
+FATE_AC3_FIXED_DEXP24-$(call ALLYES, FFMPEG WAV_DEMUXER ARESAMPLE_FILTER ATRIM_FILTER \
+                                    AC3_FIXED_ENCODER FRAMECRC_MUXER \
+                                    AC3_MUXER AC3_DEMUXER AC3_FIXED_DECODER \
+                                    PCM_S16LE_DECODER PCM_S16LE_ENCODER \
+                                    FILE_PROTOCOL) += fate-ac3-fixed-dexp24
+fate-ac3-fixed-dexp24: tests/data/fate/ac3-fixed-dexp24.ac3
+fate-ac3-fixed-dexp24: CMD = framecrc -auto_conversion_filters -c ac3_fixed \
+                                   -i $(TARGET_PATH)/tests/data/fate/ac3-fixed-dexp24.ac3 \
+                                   -af atrim=start_sample=264192
+
 FATE_EAC3-$(call ALLYES, EAC3_DEMUXER EAC3_MUXER EAC3_CORE_BSF) += fate-eac3-core-bsf
 fate-eac3-core-bsf: CMD = md5pipe -i $(TARGET_SAMPLES)/eac3/the_great_wall_7.1.eac3 -c:a copy -bsf:a eac3_core -fflags +bitexact -f eac3
 fate-eac3-core-bsf: CMP = oneline
 fate-eac3-core-bsf: REF = b704bf851e99b7442e9bed368b60e6ca
 
 FATE_SAMPLES_AVCONV += $(FATE_AC3-yes) $(FATE_EAC3-yes)
+FATE_FFMPEG += $(FATE_AC3_FIXED_DEXP24-yes)
 
-fate-ac3: $(FATE_AC3-yes) $(FATE_EAC3-yes)
+fate-ac3: $(FATE_AC3-yes) $(FATE_EAC3-yes) $(FATE_AC3_FIXED_DEXP24-yes)
diff --git a/tests/ref/fate/ac3-fixed-dexp24 b/tests/ref/fate/ac3-fixed-dexp24
new file mode 100644
index 0000000000..857d6c20ec
--- /dev/null
+++ b/tests/ref/fate/ac3-fixed-dexp24
@@ -0,0 +1,6 @@
+#tb 0: 1/44100
+#media_type 0: audio
+#codec_id 0: pcm_s16le
+#sample_rate 0: 44100
+#channel_layout_name 0: stereo
+0,     264134,     264134,     1536,     6144, 0xff5a9359
-- 
2.52.0

_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]