[FFmpeg-devel] [PATCH 1/3] lavc/pthread_frame: protect read state access in setup finish function

[wm4]

On Fri, 13 Jan 2017 12:19:14 -0500
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> Hi,
> 
> On Fri, Jan 13, 2017 at 9:39 AM, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > On Fri, 13 Jan 2017 09:07:48 -0500
> > "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
> >  
> > > Hi,
> > >
> > > On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u at pkh.me> wrote:
> > >
> > > From: Clément Bœsch <cboesch at gopro.com>
> > >
> > > ---
> > >  libavcodec/pthread_frame.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > > index 7ef5e9f6be..cb6d76284e 100644
> > > --- a/libavcodec/pthread_frame.c
> > > +++ b/libavcodec/pthread_frame.c
> > > @@ -509,11 +509,11 @@ void ff_thread_finish_setup(AVCodecContext  
> > *avctx) {  
> > >
> > >      if (!(avctx->active_thread_type&FF_THREAD_FRAME)) return;
> > >
> > > +    pthread_mutex_lock(&p->progress_mutex);
> > >      if(p->state == STATE_SETUP_FINISHED){
> > >          av_log(avctx, AV_LOG_WARNING, "Multiple ff_thread_finish_setup()
> > > calls\n");
> > >      }
> > >
> > > -    pthread_mutex_lock(&p->progress_mutex);
> > >
> > >
> > > This has a problem in that we're potentially calling an I/O function that
> > > potentially directly enters application space with a lock held.  
> >
> > I think the user should be taking care of this (or just don't spam
> > av_log in a performance critical function). The log call here seems
> > more like an internal warning though, and I've never seen it happening.
> >  
> 
> But it's easy to resolve right?
> 
> lock();
> int state = p->state;
> ..
> unlock();
> if (state == ..)
>   av_log(..);

I still don't see what the problem is. Why make it more complex than
necessary?