avcodec_flush_buffers() not resetting E-AC-3 decoder

[Peter Krefting]

Summary of the bug:
I am reusing the same AVCodecContext to decode several parallel E-AC-3 audio streams. As per documentation I am calling avcodec_flush_buffers() between each decoding run. This seems to work for most codecs, but when used with the E-AC-3 decoder the output is not identical between runs

How to reproduce:

#include <libavcodec/avcodec.h>
#include <libavformat/avformat.h>
#include <stdio.h>
#include <assert.h>

int main(int argc, char *argv[])
{
    const AVCodec *codec = avcodec_find_decoder(AV_CODEC_ID_EAC3);
    AVCodecContext *decoder = avcodec_alloc_context3(codec);
    avcodec_open2(decoder, codec, NULL);
    assert(decoder->sample_fmt == AV_SAMPLE_FMT_FLTP);

    for (int l = 0; l < 2; ++ l)
    {
        printf("[%d] Reading file\n", l);
        avcodec_flush_buffers(decoder);
        AVFormatContext *avf_ctx = NULL;
        avformat_open_input(&avf_ctx, "file:i28553p303t610156395819694.ac3", NULL, NULL);
        avformat_find_stream_info(avf_ctx, NULL);

        AVPacket avpkt = { 0 };
        while (av_read_frame(avf_ctx, &avpkt) == 0)
        {
            avcodec_send_packet(decoder, &avpkt);
            AVFrame *frame = av_frame_alloc();
            avcodec_receive_frame(decoder, frame);
            printf("Got %d samples in %d channels\n", frame->nb_samples, frame->channels);

            for (int i = 0; i < frame->nb_samples; ++ i)
            {
                printf("%4d:", i);
                for (int j = 0; j < frame->channels; ++ j)
                {
                    printf(" [%d]%f", j, ((float *) frame->data[j])[i]);
                }
                printf("\n");
            }

            av_frame_free(&frame);
        }
        avformat_close_input(&avf_ctx);
    }

    avcodec_free_context(&decoder);
    return 0;
}

The test file is available here: ​https://e.pcloud.link/publink/show?code=XZS8y1ZlEryJIDYU0yIzDTUhw1sDfHDRtUV and contains a ten-second extract of an audio track from an MPEGTS container, captured off-air while looking at a problem where the encoder suddenly only outputs silence.

I expect the output to be identical on the first and second run, but that is not what I am seeing:

$ ./decode |grep -A5 'Reading file'
[eac3 @ 0x55fc4b0a5ec0] Estimating duration from bitrate, this may be inaccurate
[0] Reading file
Got 1536 samples in 6 channels
   0: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
   1: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
   2: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
   3: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
[eac3 @ 0x55fc4b11a840] Estimating duration from bitrate, this may be inaccurate
--
[1] Reading file
Got 1536 samples in 6 channels
   0: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
   1: [0]-0.000000 [1]0.000000 [2]-0.000000 [3]-0.000000 [4]-0.000000 [5]-0.000000
   2: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
   3: [0]-0.000000 [1]-0.000000 [2]-0.000000 [3]-0.000000 [4]-0.000000 [5]-0.000000

In real life we are also interleaving other E-AC-3 audio streams from the same MPEGTS between decodes, and see that the initial decoded samples differ even more than when just redecoding the same sample over and over.

[Peter Krefting]

We are also seeing a similar issue in an unrelated case using the HEVC decoder (AV_CODEC_ID_H265). When multiplexing several streams into the same AVCodecContext object, calling avcodec_flush_buffers() between each call, we sometimes fail to get frames decoded, but if we change to having a dedicated AVCodecContext for each of the streams it decodes fine.

[Peter Krefting]

Also verified that the I see the same issue in the 6.1 release version. Updating the build framework is fairly heavy process, and I have not tested the latest git master.

[Balling]

Is this still a problem?

and see that the initial decoded samples

Can you add that sample? What is this, -0 and +0 issue... It is not even a real issue technically.

[James]

Does the following work for you?
There may have been a reason the ac3 decoder did not include a flush() method, but perhaps it was simply forgotten.

diff --git a/libavcodec/ac3dec.c b/libavcodec/ac3dec.c
index 3cc20f32a9..3490e02c89 100644
--- a/libavcodec/ac3dec.c
+++ b/libavcodec/ac3dec.c
@@ -252,6 +252,17 @@ static av_cold int ac3_decode_init(AVCodecContext *avctx)
     return 0;
 }

+static av_cold void ac3_decode_flush(AVCodecContext *avctx)
+{
+    AC3DecodeContext *s = avctx->priv_data;
+
+    memset(s + offsetof(AC3DecodeContext, frame_type), 0,
+           sizeof(*s) - offsetof(AC3DecodeContext, frame_type));
+
+    AC3_RENAME(ff_kbd_window_init)(s->window, 5.0, 256);
+    av_lfg_init(&s->dith_state, 0);
+}
+
 /**
  * Parse the 'sync info' and 'bit stream info' from the AC-3 bitstream.
  * GetBitContext within AC3DecodeContext must point to
diff --git a/libavcodec/ac3dec.h b/libavcodec/ac3dec.h
index 98de7b5abf..cfaad446ab 100644
--- a/libavcodec/ac3dec.h
+++ b/libavcodec/ac3dec.h
@@ -75,8 +75,29 @@ typedef struct AC3DecodeContext {
     AVCodecContext *avctx;                  ///< parent context
     GetBitContext gbc;                      ///< bitstream reader

+    AVTXContext *tx_128, *tx_256;
+    av_tx_fn tx_fn_128, tx_fn_256;
+
+///@name Optimization
+    BswapDSPContext bdsp;
+#if USE_FIXED
+    AVFixedDSPContext *fdsp;
+#else
+    AVFloatDSPContext *fdsp;
+#endif
+    AC3DSPContext ac3dsp;
+    FmtConvertContext fmt_conv;             ///< optimized conversion functions
+///@}
+
+    INTFLOAT *xcfptr[AC3_MAX_CHANNELS];
+    INTFLOAT *dlyptr[AC3_MAX_CHANNELS];
+
+    AVChannelLayout downmix_layout;
+
+// Start of flushable fields
 ///@name Bit stream information
 ///@{
+    // frame_type must be first
     int frame_type;                         ///< frame type                             (strmtyp)
     int substreamid;                        ///< substream identification
     int superframe_size;                    ///< current superframe size, in bytes
@@ -222,24 +243,9 @@ typedef struct AC3DecodeContext {

 ///@name IMDCT
     int block_switch[AC3_MAX_CHANNELS];     ///< block switch flags                     (blksw)
-    AVTXContext *tx_128, *tx_256;
-    av_tx_fn tx_fn_128, tx_fn_256;
-///@}
-
-///@name Optimization
-    BswapDSPContext bdsp;
-#if USE_FIXED
-    AVFixedDSPContext *fdsp;
-#else
-    AVFloatDSPContext *fdsp;
-#endif
-    AC3DSPContext ac3dsp;
-    FmtConvertContext fmt_conv;             ///< optimized conversion functions
 ///@}

     SHORTFLOAT *outptr[AC3_MAX_CHANNELS];
-    INTFLOAT *xcfptr[AC3_MAX_CHANNELS];
-    INTFLOAT *dlyptr[AC3_MAX_CHANNELS];

 ///@name Aligned arrays
     DECLARE_ALIGNED(16, int,   fixed_coeffs)[AC3_MAX_CHANNELS][AC3_MAX_COEFS];       ///< fixed-point transform coefficients
@@ -252,7 +258,7 @@ typedef struct AC3DecodeContext {
     DECLARE_ALIGNED(32, SHORTFLOAT, output_buffer)[EAC3_MAX_CHANNELS][AC3_BLOCK_SIZE * 6];  ///< final output buffer
 ///@}

-    AVChannelLayout downmix_layout;
+// End of flushable fields
 } AC3DecodeContext;

 /**
diff --git a/libavcodec/ac3dec_fixed.c b/libavcodec/ac3dec_fixed.c
index e0db9b2260..e284140e74 100644
--- a/libavcodec/ac3dec_fixed.c
+++ b/libavcodec/ac3dec_fixed.c
@@ -181,6 +181,7 @@ const FFCodec ff_ac3_fixed_decoder = {
     .p.priv_class   = &ac3_decoder_class,
     .priv_data_size = sizeof (AC3DecodeContext),
     .init           = ac3_decode_init,
+    .flush          = ac3_decode_flush,
     .close          = ac3_decode_end,
     FF_CODEC_DECODE_CB(ac3_decode_frame),
     .p.capabilities = AV_CODEC_CAP_CHANNEL_CONF |
diff --git a/libavcodec/ac3dec_float.c b/libavcodec/ac3dec_float.c
index d94070bc0c..60d3472ca6 100644
--- a/libavcodec/ac3dec_float.c
+++ b/libavcodec/ac3dec_float.c
@@ -87,6 +87,7 @@ const FFCodec ff_eac3_decoder = {
     .p.id           = AV_CODEC_ID_EAC3,
     .priv_data_size = sizeof (AC3DecodeContext),
     .init           = ac3_decode_init,
+    .flush          = ac3_decode_flush,
     .close          = ac3_decode_end,
     FF_CODEC_DECODE_CB(ac3_decode_frame),
     .p.capabilities = AV_CODEC_CAP_CHANNEL_CONF |

[Peter Krefting]

Can you add that sample?

The test file is linked in the bug description.

What is this, -0 and +0 issue... It is not even a real issue technically.

I should have included more of the output, when running the binary with the original library we see that there are different outputs each iteration, whereas I expect getting the same output for the same input each iteration:

$ ./ffmpeg-10732 |grep '  18: '|head -n5
[eac3 @ 0x59650c9c5bc0] Estimating duration from bitrate, this may be inaccurate
  18: [0]0.000000 [1]0.000000 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
  18: [0]0.000000 [1]-0.000002 [2]0.000000 [3]0.000000 [4]0.000000 [5]-0.000002
  18: [0]0.000000 [1]-0.000002 [2]0.000000 [3]0.000000 [4]0.000000 [5]-0.000001
  18: [0]0.000000 [1]-0.000002 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000
  18: [0]0.000000 [1]-0.000002 [2]0.000000 [3]0.000000 [4]0.000000 [5]0.000000

I will need some more time to investigate your patch, as it doesn't apply cleanly to the version we are still on, and the API changes in the current version will require some work to integrate (including the test program from the description).

[Balling]

The test file is linked in the bug description.

You also mentioned you have a better sample. I think this should be applied.

[Peter Krefting]

You also mentioned you have a better sample. I think this should be applied.

Unfortunately not. The code that is affected and that my sample code is set to imitate runs on off-air samples and produce loudness information only, it does not store the incoming streams.

The other case I mentioned was in similar code where we process multiple HEVC streams using the same decoder. In that case we detect the keyframe and pass that to the HEVC decoder to extract an image, reset the decoder, then pass another keyframe (possibly from a completely unrelated stream), extract an image, etc.

I have not yet had the time to test the provided bugfix, we are currently preparing a new release, so upgrading is impractical, but it is on my to-do list.

[Peter Krefting]

I am now in the process of updating the source tree (to ffmpeg 7.1). I applied the patch from comment:5. With the patch, the tester program now crashes with a segmentation fault inside av3_decode_flush(). Adding an if (s) around the memset() invocation is not enough to rectify this, unfortunately.

To compile the tester with 7.1, I replaced frame->channels with frame->ch_layout.nb_channels (two places), otherwise the code is as in the description above.

[James]

Replying to Peter Krefting:

I am now in the process of updating the source tree (to ffmpeg 7.1). I applied the patch from comment:5. With the patch, the tester program now crashes with a segmentation fault inside av3_decode_flush(). Adding an if (s) around the memset() invocation is not enough to rectify this, unfortunately.

To compile the tester with 7.1, I replaced frame->channels with frame->ch_layout.nb_channels (two places), otherwise the code is as in the description above.

Oh, i see where i messed up. I sent a fixed version to the mailing list. Can you test it? It's in ​https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=13875

[Peter Krefting]

Thank you, the updated patch from patchwork does indeed fix the issue I am seeing.

Is it possible to identify which codecs do not have an implementation for avcodec_flush_buffers(), we would like to re-use the objects as much as possible, for performance reasons. Clearing an object is usually cheaper than deallocation and allocating a new one.

[James]

Replying to Peter Krefting:

Thank you, the updated patch from patchwork does indeed fix the issue I am seeing.

Fixed in 045a8b15b19ec7f872fb01cfb986faeaa26cb8bb.

Is it possible to identify which codecs do not have an implementation for avcodec_flush_buffers(), we would like to re-use the objects as much as possible, for performance reasons. Clearing an object is usually cheaper than deallocation and allocating a new one.

All decoders by default support being flushed with that function. Those that need it provide an internal flush() callback to clear some decoder private state. In the case of ac3/eac3 as you found out, it was most likely forgotten.
If you find another decoder where flushing doesn't behave as it should, create another ticket.

[Balling]

For your information TrueHD and MLP recently added flush that fixed a terrible bug where at every seek it was warning about lossless absence.

5673a4842556b79a92a1ede6e9696506fd4161ad
034133a0df5f327aba36ee25db9452cda9e1a62b

[Balling]

Hi, can you check whether this bug is the same bug you had with your code ​https://trac.ffmpeg.org/ticket/11578#comment:14

[Peter Krefting]

Replying to Balling:

can you check whether this bug is the same bug you had with your code ​https://trac.ffmpeg.org/ticket/11578#comment:14

It does indeed look similar to what we saw. We are not playing back the decoded audio, only graphing the loudness, but the graphs showed short peaks immediately after calling the flush function and switching buffers, consistent with a "click" sound. We are only using the ffmpeg functionality for reading data, we feed the data directly to the decoder.

[Balling]

No, there if you do not seek there is a click. If you do to the start after click has played there is no click.