[FFmpeg-devel] [PATCH, v3] lavc/pthread_frame: update context in child thread in multi-thread mode

[Fu, Linjie]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Hendrik Leppkes
> Sent: Friday, June 28, 2019 08:56
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v3] lavc/pthread_frame: update
> context in child thread in multi-thread mode
> 
> On Thu, Jun 27, 2019 at 1:56 PM Linjie Fu <linjie.fu at intel.com> wrote:
> >
> > Currently in ff_thread_decode_frame, context is updated from child
> thread
> > to user thread, and user thread releases the context in avcodec_close()
> > when decode finishes.
> >
> > However, when resolution/format changes, ff_get_format is called, and
> > hwaccel_uninit() and hwaccel_init will be used to destroy and re-create
> > the context. Due to the async between user-thread and child-thread,
> > user-thread updates its context from child earlier than the context
> > is refreshed in child-thread. And it will lead to:
> >     1. memory leak in child-thread.
> >     2. double free in user-thread while calling avcodec_close().
> >
> > Can be reproduced with a resolution change case, and use -vframes
> > to terminate the decode between the dynamic resolution changing 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
> >
> > 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 out.yuv
> >
> > Move update_context_from_thread from ff_thread_decode_frame(user
> thread)
> > to frame_worker_thread(child thread), update the context in child thread
> > right after the context is refreshed to avoid the async issue.
> >
> 
> I think the undelying issue that Michael mentioned remains - you are
> changing the user context from a worker thread, at which point the
> user might be accessing his context simultaneously. You cannot prevent
> that with a mutex, since the user does not use your mutex.

User context could be accessed everywhere in the user thread, so changing from
worker thread will always lead to a race condition.
Seems I didn't fully understood this before.

Async feature is designed on purpose, it's hard to capture the changes in worker
thread in time and get the context updated for user thread correctly.

As the async issue exists commonly for resolution changing, it is in need for fixing.

> Additionally, the user context should reflect the state of the last
> frame that was output to the user, if a worker thread changes it
> immediately as it sees the size change, wouldn't it be possible for
> frames of the old size to be output after the context was already
> updated? That sounds like trouble to me.

I didn't quite understand it, but as another patch for vp9 shows, output frame whose size
Is different from context can still be correct.

Anyway, since it's not easy(or possible) to modify in worker thread, this won't be a problem. 

Thanks,
- linjie