| Message ID | 20180102075223.65278-1-zhangjiejun1992@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
2018-01-02 15:52 GMT+08:00 <zhangjiejun1992@gmail.com>: > From: Jiejun Zhang <zhangjiejun1992@gmail.com> > > This fixes #6940 > --- > libavcodec/audiotoolboxenc.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c > index 71885d1530..b70375a692 100644 > --- a/libavcodec/audiotoolboxenc.c > +++ b/libavcodec/audiotoolboxenc.c > @@ -48,6 +48,9 @@ typedef struct ATDecodeContext { > AudioFrameQueue afq; > int eof; > int frame_size; > + > + uint8_t* audio_data_buf; > + uint32_t audio_data_buf_size; > } ATDecodeContext; > > static UInt32 ffat_get_format_id(enum AVCodecID codec, int profile) > @@ -442,6 +445,9 @@ static av_cold int ffat_init_encoder(AVCodecContext *avctx) > > ff_af_queue_init(avctx, &at->afq); > > + at->audio_data_buf_size = 0; > + at->audio_data_buf = NULL; > + > return 0; > } > > @@ -471,7 +477,15 @@ static OSStatus ffat_encode_callback(AudioConverterRef converter, UInt32 *nb_pac > data->mBuffers[0].mDataByteSize = frame->nb_samples * > av_get_bytes_per_sample(avctx->sample_fmt) * > avctx->channels; > - data->mBuffers[0].mData = frame->data[0]; > + if (at->audio_data_buf_size < data->mBuffers[0].mDataByteSize) { > + av_log(avctx, AV_LOG_INFO, "Increasing audio data buffer size to %u", > + data->mBuffers[0].mDataByteSize); > + av_free(at->audio_data_buf); > + at->audio_data_buf_size = data->mBuffers[0].mDataByteSize; > + at->audio_data_buf = av_malloc(at->audio_data_buf_size); Need check the av_malloc result here. > + } > + memcpy(at->audio_data_buf, frame->data[0], data->mBuffers[0].mDataByteSize); > + data->mBuffers[0].mData = at->audio_data_buf; > if (*nb_packets > frame->nb_samples) > *nb_packets = frame->nb_samples; > > @@ -565,6 +579,10 @@ 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); > + if (at->audio_data_buf_size > 0) { > + at->audio_data_buf_size = 0; > + av_free(at->audio_data_buf); > + } > return 0; > } > > -- > 2.14.3 (Apple Git-98) > Thanks Steven
2018-01-02 8:52 GMT+01:00 <zhangjiejun1992@gmail.com>: > @@ -565,6 +579,10 @@ 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); > + if (at->audio_data_buf_size > 0) { > + at->audio_data_buf_size = 0; > + av_free(at->audio_data_buf); > + } The if() looks unnecessary. Could you explain what this patch changes? From a quick look, until now a buffer a was used with a calculated size x. After the patch, a buffer b with the same calculated size x is allocated, and x bytes are copied from a to b. What do I miss? Thank you, Carl Eugen
On Tue, Jan 2, 2018 at 8:03 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-01-02 8:52 GMT+01:00 <zhangjiejun1992@gmail.com>: > >> @@ -565,6 +579,10 @@ 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); >> + if (at->audio_data_buf_size > 0) { >> + at->audio_data_buf_size = 0; >> + av_free(at->audio_data_buf); >> + } > > The if() looks unnecessary. Yes. I'll remove it. Thanks for pointing it out. > Could you explain what this patch changes? > From a quick look, until now a buffer a was used with a calculated size x. > After the patch, a buffer b with the same calculated size x is allocated, > and x bytes are copied from a to b. > What do I miss? 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. Copying the data to a separate buffer solves this problem.
On Tue, 2 Jan 2018 22:27:49 +0800 Jiejun Zhang <zhangjiejun1992@gmail.com> wrote: > On Tue, Jan 2, 2018 at 8:03 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > 2018-01-02 8:52 GMT+01:00 <zhangjiejun1992@gmail.com>: > > > >> @@ -565,6 +579,10 @@ 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); > >> + if (at->audio_data_buf_size > 0) { > >> + at->audio_data_buf_size = 0; > >> + av_free(at->audio_data_buf); > >> + } > > > > The if() looks unnecessary. > > Yes. I'll remove it. Thanks for pointing it out. > > > Could you explain what this patch changes? > > From a quick look, until now a buffer a was used with a calculated size x. > > After the patch, a buffer b with the same calculated size x is allocated, > > and x bytes are copied from a to b. > > What do I miss? > > 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. Copying the data to a separate buffer solves this > problem. This should be in the commit message.
On Tue, Jan 2, 2018 at 10:37 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 2 Jan 2018 22:27:49 +0800 > Jiejun Zhang <zhangjiejun1992@gmail.com> wrote: > >> On Tue, Jan 2, 2018 at 8:03 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> > 2018-01-02 8:52 GMT+01:00 <zhangjiejun1992@gmail.com>: >> > >> >> @@ -565,6 +579,10 @@ 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); >> >> + if (at->audio_data_buf_size > 0) { >> >> + at->audio_data_buf_size = 0; >> >> + av_free(at->audio_data_buf); >> >> + } >> > >> > The if() looks unnecessary. >> >> Yes. I'll remove it. Thanks for pointing it out. >> >> > Could you explain what this patch changes? >> > From a quick look, until now a buffer a was used with a calculated size x. >> > After the patch, a buffer b with the same calculated size x is allocated, >> > and x bytes are copied from a to b. >> > What do I miss? >> >> 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. Copying the data to a separate buffer solves this >> problem. > > This should be in the commit message. Done. Submitting v2
diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c index 71885d1530..b70375a692 100644 --- a/libavcodec/audiotoolboxenc.c +++ b/libavcodec/audiotoolboxenc.c @@ -48,6 +48,9 @@ typedef struct ATDecodeContext { AudioFrameQueue afq; int eof; int frame_size; + + uint8_t* audio_data_buf; + uint32_t audio_data_buf_size; } ATDecodeContext; static UInt32 ffat_get_format_id(enum AVCodecID codec, int profile) @@ -442,6 +445,9 @@ static av_cold int ffat_init_encoder(AVCodecContext *avctx) ff_af_queue_init(avctx, &at->afq); + at->audio_data_buf_size = 0; + at->audio_data_buf = NULL; + return 0; } @@ -471,7 +477,15 @@ static OSStatus ffat_encode_callback(AudioConverterRef converter, UInt32 *nb_pac data->mBuffers[0].mDataByteSize = frame->nb_samples * av_get_bytes_per_sample(avctx->sample_fmt) * avctx->channels; - data->mBuffers[0].mData = frame->data[0]; + if (at->audio_data_buf_size < data->mBuffers[0].mDataByteSize) { + av_log(avctx, AV_LOG_INFO, "Increasing audio data buffer size to %u", + data->mBuffers[0].mDataByteSize); + av_free(at->audio_data_buf); + at->audio_data_buf_size = data->mBuffers[0].mDataByteSize; + at->audio_data_buf = av_malloc(at->audio_data_buf_size); + } + memcpy(at->audio_data_buf, frame->data[0], data->mBuffers[0].mDataByteSize); + data->mBuffers[0].mData = at->audio_data_buf; if (*nb_packets > frame->nb_samples) *nb_packets = frame->nb_samples; @@ -565,6 +579,10 @@ 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); + if (at->audio_data_buf_size > 0) { + at->audio_data_buf_size = 0; + av_free(at->audio_data_buf); + } return 0; }
From: Jiejun Zhang <zhangjiejun1992@gmail.com> This fixes #6940 --- libavcodec/audiotoolboxenc.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)