[FFmpeg-devel,v4] lavc/audiotoolboxenc: fix noise in encoded audio

Submitted by zhangjiejun1992@gmail.com on Jan. 3, 2018, 4:54 a.m.

Details

Message ID 20180103045420.11869-1-zhangjiejun1992@gmail.com
State New
Headers show

Commit Message

zhangjiejun1992@gmail.com Jan. 3, 2018, 4:54 a.m.
From: Jiejun Zhang <zhangjiejun1992@gmail.com>

This fixes #6940

Although undocumented, AudioToolbox seems to require the data supplied
by the callback (i.e. ffat_encode_callback) being unchanged until the
next time the callback is called. In the old implementation, the
AVBuffer backing the frame is recycled after the frame is freed, and
somebody else (maybe the decoder) will write into the AVBuffer and
change the data. AudioToolbox then encodes some wrong data and noise
is produced. Retaining a frame reference solves this problem.
---
 libavcodec/audiotoolboxenc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Bang He Jan. 3, 2018, 6:24 a.m.
maybe you should  return 1, not return ret

On Wed, Jan 3, 2018 at 12:54 PM, <zhangjiejun1992@gmail.com> wrote:

> From: Jiejun Zhang <zhangjiejun1992@gmail.com>
>
> This fixes #6940
>
> Although undocumented, AudioToolbox seems to require the data supplied
> by the callback (i.e. ffat_encode_callback) being unchanged until the
> next time the callback is called. In the old implementation, the
> AVBuffer backing the frame is recycled after the frame is freed, and
> somebody else (maybe the decoder) will write into the AVBuffer and
> change the data. AudioToolbox then encodes some wrong data and noise
> is produced. Retaining a frame reference solves this problem.
> ---
>  libavcodec/audiotoolboxenc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c
> index 71885d1530..4d8130af96 100644
> --- a/libavcodec/audiotoolboxenc.c
> +++ b/libavcodec/audiotoolboxenc.c
> @@ -48,6 +48,8 @@ typedef struct ATDecodeContext {
>      AudioFrameQueue afq;
>      int eof;
>      int frame_size;
> +
> +    AVFrame* encoding_frame;
>  } ATDecodeContext;
>
>  static UInt32 ffat_get_format_id(enum AVCodecID codec, int profile)
> @@ -442,6 +444,10 @@ static av_cold int ffat_init_encoder(AVCodecContext
> *avctx)
>
>      ff_af_queue_init(avctx, &at->afq);
>
> +    at->encoding_frame = av_frame_alloc();
> +    if (!at->encoding_frame)
> +        return AVERROR(ENOMEM);
> +
>      return 0;
>  }
>
> @@ -453,6 +459,7 @@ static OSStatus ffat_encode_callback(AudioConverterRef
> converter, UInt32 *nb_pac
>      AVCodecContext *avctx = inctx;
>      ATDecodeContext *at = avctx->priv_data;
>      AVFrame *frame;
> +    int ret;
>
>      if (!at->frame_queue.available) {
>          if (at->eof) {
> @@ -475,6 +482,12 @@ static OSStatus ffat_encode_callback(AudioConverterRef
> converter, UInt32 *nb_pac
>      if (*nb_packets > frame->nb_samples)
>          *nb_packets = frame->nb_samples;
>
> +    av_frame_unref(at->encoding_frame);
> +    if ((ret = av_frame_ref(at->encoding_frame, frame)) < 0) {
> +        *nb_packets = 0;
> +        return ret;
> +    }
> +
>      ff_bufqueue_add(avctx, &at->used_frame_queue, frame);
>
>      return 0;
> @@ -565,6 +578,7 @@ static av_cold int ffat_close_encoder(AVCodecContext
> *avctx)
>      ff_bufqueue_discard_all(&at->frame_queue);
>      ff_bufqueue_discard_all(&at->used_frame_queue);
>      ff_af_queue_close(&at->afq);
> +    av_frame_free(&at->encoding_frame);
>      return 0;
>  }
>
> --
> 2.14.3 (Apple Git-98)
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
zhangjiejun1992@gmail.com Jan. 3, 2018, 8:10 a.m.
On Wed, Jan 3, 2018 at 2:24 PM, Bang He <hezhanbang@gmail.com> wrote:
> maybe you should  return 1, not return ret

returning ret in callback will make AudioConverterFillComplexBuffer
return ret. so i think returning ret is better. furthermore 1 is not
treated as an error code in the current implementation of ffat_encode.
James Almer Jan. 3, 2018, 8:34 p.m.
On 1/3/2018 1:54 AM, zhangjiejun1992@gmail.com wrote:
> From: Jiejun Zhang <zhangjiejun1992@gmail.com>
> 
> This fixes #6940
> 
> Although undocumented, AudioToolbox seems to require the data supplied
> by the callback (i.e. ffat_encode_callback) being unchanged until the
> next time the callback is called. In the old implementation, the
> AVBuffer backing the frame is recycled after the frame is freed, and
> somebody else (maybe the decoder) will write into the AVBuffer and
> change the data. AudioToolbox then encodes some wrong data and noise
> is produced. Retaining a frame reference solves this problem.
> ---
>  libavcodec/audiotoolboxenc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Applied.

Patch hide | download patch | download mbox

diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c
index 71885d1530..4d8130af96 100644
--- a/libavcodec/audiotoolboxenc.c
+++ b/libavcodec/audiotoolboxenc.c
@@ -48,6 +48,8 @@  typedef struct ATDecodeContext {
     AudioFrameQueue afq;
     int eof;
     int frame_size;
+
+    AVFrame* encoding_frame;
 } ATDecodeContext;
 
 static UInt32 ffat_get_format_id(enum AVCodecID codec, int profile)
@@ -442,6 +444,10 @@  static av_cold int ffat_init_encoder(AVCodecContext *avctx)
 
     ff_af_queue_init(avctx, &at->afq);
 
+    at->encoding_frame = av_frame_alloc();
+    if (!at->encoding_frame)
+        return AVERROR(ENOMEM);
+
     return 0;
 }
 
@@ -453,6 +459,7 @@  static OSStatus ffat_encode_callback(AudioConverterRef converter, UInt32 *nb_pac
     AVCodecContext *avctx = inctx;
     ATDecodeContext *at = avctx->priv_data;
     AVFrame *frame;
+    int ret;
 
     if (!at->frame_queue.available) {
         if (at->eof) {
@@ -475,6 +482,12 @@  static OSStatus ffat_encode_callback(AudioConverterRef converter, UInt32 *nb_pac
     if (*nb_packets > frame->nb_samples)
         *nb_packets = frame->nb_samples;
 
+    av_frame_unref(at->encoding_frame);
+    if ((ret = av_frame_ref(at->encoding_frame, frame)) < 0) {
+        *nb_packets = 0;
+        return ret;
+    }
+
     ff_bufqueue_add(avctx, &at->used_frame_queue, frame);
 
     return 0;
@@ -565,6 +578,7 @@  static av_cold int ffat_close_encoder(AVCodecContext *avctx)
     ff_bufqueue_discard_all(&at->frame_queue);
     ff_bufqueue_discard_all(&at->used_frame_queue);
     ff_af_queue_close(&at->afq);
+    av_frame_free(&at->encoding_frame);
     return 0;
 }