| Message ID | 1561580692-32634-1-git-send-email-linjie.fu@intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 26, 2019 at 04:24:52PM -0400, Linjie Fu wrote: > Currently in ff_thread_decode_frame, context is updated from child thread > to main thread, and main thread releases the context in avcodec_close() > when decode finishes. > > However, when resolution/format change in vp9, ff_get_format was called, > and hwaccel_uninit() and hwaccel_init will be used to destroy and re-create > the context. Due to the async between main-thread and child-thread, > main-thread updated its context from child earlier than the context was > refreshed in child-thread. And it will lead to: > 1. memory leak in child-thread. > 2. double free in main-thread while calling avcodec_close(). > > Can be reproduced with a resolution change case in vp9, and use -vframes > to terminate the decode between the dynamic resolution change frames: > > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -v > verbose -i ./test2360_1672_4980.ivf -pix_fmt p010le -f rawvideo -vsync > passthrough -vframes 6 -y out.yuv > > Move update_context_from_thread from ff_thread_decode_frame(main thread) > to frame_worker_thread(child thread), update the context in child thread > right after the context was updated to avoid the async issue. > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > Request for Comments, not quite familiar with the constraints. > Thanks in advance. i dont think i fully understand the problem you are trying to fix but this patch looks like it writes into the users context without any lock while the user can access it. Thats looks like a race condition unless iam missing something What is very noticable though is that you seem to talk about vp9 why is this vp9 specific and does not affect other codecs ? thanks [...]
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Michael Niedermayer > Sent: Thursday, June 27, 2019 00:29 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH, RFC, v2] lavc/phtread_frame: update > context in child thread in multi-thread mode > > On Wed, Jun 26, 2019 at 04:24:52PM -0400, Linjie Fu wrote: > > Currently in ff_thread_decode_frame, context is updated from child > thread > > to main thread, and main thread releases the context in avcodec_close() > > when decode finishes. > > > > However, when resolution/format change in vp9, ff_get_format was called, > > and hwaccel_uninit() and hwaccel_init will be used to destroy and re- > create > > the context. Due to the async between main-thread and child-thread, > > main-thread updated its context from child earlier than the context was > > refreshed in child-thread. And it will lead to: > > 1. memory leak in child-thread. > > 2. double free in main-thread while calling avcodec_close(). > > > > Can be reproduced with a resolution change case in vp9, and use -vframes > > to terminate the decode between the dynamic resolution change frames: > > > > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -v > > verbose -i ./test2360_1672_4980.ivf -pix_fmt p010le -f rawvideo -vsync > > passthrough -vframes 6 -y out.yuv > > > > Move update_context_from_thread from ff_thread_decode_frame(main > thread) > > to frame_worker_thread(child thread), update the context in child thread > > right after the context was updated to avoid the async issue. > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > --- > > Request for Comments, not quite familiar with the constraints. > > Thanks in advance. > > i dont think i fully understand the problem you are trying to fix but > this patch looks like it writes into the users context without any > lock while the user can access it. > Thats looks like a race condition unless I am missing something Yes that's what I'm concerned and seeking for advice. One possible way I thought is to add a new lock, and lock in both frame_worker_thread and submit_packet(user) wwhen it attmepts to update context from user to child thread. > What is very noticable though is that you seem to talk about vp9 > why is this vp9 specific and does not affect other codecs ? Actually it's not codec specific. It happens as long as context refreshed because of resolution/format change in child thread but failed to update in main thread correctly. Same issue exists in h264 as well if decode terminate during resolution changing (-vframes 45 in this example): ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -v verbose -i ./reinit-large_420_8-to-small_420_8.h264 -pix_fmt nv12 -f rawvideo -vsync passthrough -vframes 45 -y md5.yuv http://fate-suite.ffmpeg.org/h264/reinit-large_420_8-to-small_420_8.h264 And this patch fixed it as well. It seems I should not restrict this issue to vp9 in commit message. - Linjie
diff --git a/libavcodec/internal.h b/libavcodec/internal.h index 5096ffa..9f4ed0b 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -162,6 +162,11 @@ typedef struct AVCodecInternal { void *thread_ctx; + /** + * Main thread AVCodecContext pointer + */ + void *main_thread; + DecodeSimpleContext ds; DecodeFilterContext filter; diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 36ac0ac..2730a8c 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -140,6 +140,8 @@ typedef struct FrameThreadContext { #define THREAD_SAFE_CALLBACKS(avctx) \ ((avctx)->thread_safe_callbacks || (avctx)->get_buffer2 == avcodec_default_get_buffer2) +static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src, int for_user); + static void async_lock(FrameThreadContext *fctx) { pthread_mutex_lock(&fctx->async_mutex); @@ -157,7 +159,6 @@ static void async_unlock(FrameThreadContext *fctx) pthread_cond_broadcast(&fctx->async_cond); pthread_mutex_unlock(&fctx->async_mutex); } - /** * Codec worker thread. * @@ -200,6 +201,10 @@ static attribute_align_arg void *frame_worker_thread(void *arg) p->got_frame = 0; p->result = codec->decode(avctx, p->frame, &p->got_frame, &p->avpkt); + if (p->avctx->internal->main_thread) + update_context_from_thread((AVCodecContext *)p->avctx->internal->main_thread, + p->avctx, 1); + if ((p->result < 0 || !p->got_frame) && p->frame->buf[0]) { if (avctx->internal->allocate_progress) av_log(avctx, AV_LOG_ERROR, "A frame threaded decoder did not " @@ -540,8 +545,6 @@ int ff_thread_decode_frame(AVCodecContext *avctx, if (finished >= avctx->thread_count) finished = 0; } while (!avpkt->size && !*got_picture_ptr && err >= 0 && finished != fctx->next_finished); - update_context_from_thread(avctx, p->avctx, 1); - if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0; fctx->next_finished = finished; @@ -728,6 +731,8 @@ int ff_frame_thread_init(AVCodecContext *avctx) FrameThreadContext *fctx; int i, err = 0; + avctx->internal->main_thread = avctx; + if (!thread_count) { int nb_cpus = av_cpu_count(); #if FF_API_DEBUG_MV @@ -800,6 +805,7 @@ int ff_frame_thread_init(AVCodecContext *avctx) *copy->internal = *src->internal; copy->internal->thread_ctx = p; copy->internal->last_pkt_props = &p->avpkt; + copy->internal->main_thread = avctx; if (!i) { src = copy;
Currently in ff_thread_decode_frame, context is updated from child thread to main thread, and main thread releases the context in avcodec_close() when decode finishes. However, when resolution/format change in vp9, ff_get_format was called, and hwaccel_uninit() and hwaccel_init will be used to destroy and re-create the context. Due to the async between main-thread and child-thread, main-thread updated its context from child earlier than the context was refreshed in child-thread. And it will lead to: 1. memory leak in child-thread. 2. double free in main-thread while calling avcodec_close(). Can be reproduced with a resolution change case in vp9, and use -vframes to terminate the decode between the dynamic resolution change frames: ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -v verbose -i ./test2360_1672_4980.ivf -pix_fmt p010le -f rawvideo -vsync passthrough -vframes 6 -y out.yuv Move update_context_from_thread from ff_thread_decode_frame(main thread) to frame_worker_thread(child thread), update the context in child thread right after the context was updated to avoid the async issue. Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- Request for Comments, not quite familiar with the constraints. Thanks in advance. libavcodec/internal.h | 5 +++++ libavcodec/pthread_frame.c | 12 +++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-)