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

[Clément Bœsch]

On Fri, Jan 13, 2017 at 07:13:10PM +0100, wm4 wrote:
> 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?

2017-01-13 21:02:31     @ubitux wm4: BBB: so patch is ok as is, or i need to split the av_log out?
2017-01-13 21:03:47     @BBB    no it’s fine
2017-01-13 21:03:50     @BBB    just keep it
2017-01-13 21:04:02     @BBB    if assumption is that doing what I said is a bug, then no need to adjust to it
2017-01-13 21:08:20     +wm4    ok with me

patch applied

thanks

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170116/8318cf25/attachment.sig>