Re: [FFmpeg-devel] [PATCH] lavc/audiotoolboxenc: fix noise in encoded audio

[Jiejun Zhang]

On Tue, Jan 2, 2018 at 10:37 PM, wm4  wrote:
> On Tue, 2 Jan 2018 22:27:49 +0800
> Jiejun Zhang  wrote:
>
>> On Tue, Jan 2, 2018 at 8:03 PM, Carl Eugen Hoyos  wrote:
>> > 2018-01-02 8:52 GMT+01:00  :
>> >
>> >> @@ -565,6 +579,10 @@ static av_cold int ffat_close_encoder(AVCodecContext 
>> >> *avctx)
>> >>  ff_bufqueue_discard_all(>frame_queue);
>> >>  ff_bufqueue_discard_all(>used_frame_queue);
>> >>  ff_af_queue_close(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/audiotoolboxenc: fix noise in encoded audio

[wm4]

On Tue, 2 Jan 2018 22:27:49 +0800
Jiejun Zhang  wrote:

> On Tue, Jan 2, 2018 at 8:03 PM, Carl Eugen Hoyos  wrote:
> > 2018-01-02 8:52 GMT+01:00  :
> >  
> >> @@ -565,6 +579,10 @@ static av_cold int ffat_close_encoder(AVCodecContext 
> >> *avctx)
> >>  ff_bufqueue_discard_all(>frame_queue);
> >>  ff_bufqueue_discard_all(>used_frame_queue);
> >>  ff_af_queue_close(>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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/audiotoolboxenc: fix noise in encoded audio

[Jiejun Zhang]

On Tue, Jan 2, 2018 at 8:03 PM, Carl Eugen Hoyos  wrote:
> 2018-01-02 8:52 GMT+01:00  :
>
>> @@ -565,6 +579,10 @@ static av_cold int ffat_close_encoder(AVCodecContext 
>> *avctx)
>>  ff_bufqueue_discard_all(>frame_queue);
>>  ff_bufqueue_discard_all(>used_frame_queue);
>>  ff_af_queue_close(>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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/audiotoolboxenc: fix noise in encoded audio

[Carl Eugen Hoyos]

2018-01-02 8:52 GMT+01:00  :

> @@ -565,6 +579,10 @@ static av_cold int ffat_close_encoder(AVCodecContext 
> *avctx)
>  ff_bufqueue_discard_all(>frame_queue);
>  ff_bufqueue_discard_all(>used_frame_queue);
>  ff_af_queue_close(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/audiotoolboxenc: fix noise in encoded audio

[Steven Liu]

2018-01-02 16:59 GMT+08:00  :
> From: Jiejun Zhang 
>
> This fixes #6940
> ---
>  libavcodec/audiotoolboxenc.c | 34 +-
>  1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c
> index 71885d1530..0c1e5feadc 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, >afq);
>
> +at->audio_data_buf_size = 0;
> +at->audio_data_buf = NULL;
> +
>  return 0;
>  }
>
> @@ -465,13 +471,27 @@ static OSStatus ffat_encode_callback(AudioConverterRef 
> converter, UInt32 *nb_pac
>  }
>
>  frame = ff_bufqueue_get(>frame_queue);
> -
> +int audio_data_size = frame->nb_samples *
> +  av_get_bytes_per_sample(avctx->sample_fmt) *
> +  avctx->channels;
> +if (at->audio_data_buf_size < audio_data_size) {
> +av_log(avctx, AV_LOG_INFO, "Increasing audio data buffer size to 
> %d\n",
> +   audio_data_size);
> +av_free(at->audio_data_buf);
> +at->audio_data_buf_size = audio_data_size;
> +at->audio_data_buf = av_malloc(at->audio_data_buf_size);
> +if (!at->audio_data_buf) {
> +at->audio_data_buf_size = 0;
> +data->mNumberBuffers = 0;
> +*nb_packets = 0;
> +return AVERROR(ENOMEM);
> +}
> +}
>  data->mNumberBuffers  = 1;
>  data->mBuffers[0].mNumberChannels = avctx->channels;
> -data->mBuffers[0].mDataByteSize   = frame->nb_samples *
> -
> av_get_bytes_per_sample(avctx->sample_fmt) *
> -avctx->channels;
> -data->mBuffers[0].mData   = frame->data[0];
> +data->mBuffers[0].mDataByteSize   = audio_data_size;
> +data->mBuffers[0].mData   = at->audio_data_buf;
> +memcpy(at->audio_data_buf, frame->data[0], 
> data->mBuffers[0].mDataByteSize);
>  if (*nb_packets > frame->nb_samples)
>  *nb_packets = frame->nb_samples;
>
> @@ -565,6 +585,10 @@ static av_cold int ffat_close_encoder(AVCodecContext 
> *avctx)
>  ff_bufqueue_discard_all(>frame_queue);
>  ff_bufqueue_discard_all(>used_frame_queue);
>  ff_af_queue_close(>afq);
> +if (at->audio_data_buf_size > 0) {
> +at->audio_data_buf_size = 0;
> +av_free(at->audio_data_buf);
> +}
>  return 0;
>  }


LGTM


Thanks

Steven
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/audiotoolboxenc: fix noise in encoded audio

[Steven Liu]

2018-01-02 15:52 GMT+08:00  :
> From: Jiejun Zhang 
>
> 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, >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(>frame_queue);
>  ff_bufqueue_discard_all(>used_frame_queue);
>  ff_af_queue_close(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel