Re: [FFmpeg-devel] [PATCH v3] libavformat/mov: fix multiple stsd handling of files with edit list, fix #6584
On Fri, Aug 18, 2017 at 8:16 AM, Michael Niedermayer wrote: > > seeking still fails with this and the sample from #6584 is that > intended or a seperate issue ? I didn't see any failure using "./ffmpeg -ss 00:00:10" (and also other time from 0s to 20s). How to reproduce a seeking failure? ___ 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
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(&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.
___
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
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(&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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2] lavc/audiotoolboxenc: fix noise in encoded audio
> > Can't you instead create a new reference for the frame buffer? Or will > making it non writable break things further into the process? It would > save you a memcpy per frame. Great idea. It works. Making it non-writable should be enough. I'm submitting v3. Please take a look. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v3] lavc/audiotoolboxenc: fix noise in encoded audio
On Wed, Jan 3, 2018 at 10:02 AM, James Almer wrote:
> On 1/2/2018 1:24 PM, zhangjiejun1...@gmail.com wrote:
>> From: Jiejun Zhang
>>
>> 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 | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c
>> index 71885d1530..5d3a746348 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,10 @@ 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)
>> +return ret;
>
> Wouldn't you have to set nb_packets to 0 in case of failure? And for a
> non libav* callback function maybe this shouldn't return an AVERROR(),
> but just 1 instead.
Yeah, will fix it. For the return value, according to Apple's doc: "If
your callback returns an error, it must return zero packets of data.
Upon receiving zero packets, the AudioConverterFillComplexBuffer
function delivers any pending output, stops producing further output,
and returns the error code.", the return value will be finally
returned to ffat_encode. So I think it's fine to return an AVERROR
here, sounds good?
>
> Also, look at audiotoolboxdec.c, where the reference (packet there
> instead of frame) is created right before calling
> AudioConverterFillComplexBuffer(), as it can fail. Maybe you can do
> something like that instead.
>
This might not be possible since a buffer queue is used while
encoding. There's no way to know which frame is currently being
encoded outside the callback. According to a previous commit the
callback is not always called by AudioConverterFillComplexBuffer.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v4] lavc/audiotoolboxenc: fix noise in encoded audio
On Wed, Jan 3, 2018 at 2:24 PM, Bang He 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
