| Message ID | CAEEMt2=q-Kfyzy8HQkoHKc2nBrzfaH9YVDNDoK7ZRcCY3TmAiw@mail.gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 13 Jan 2017 09:07:48 -0500 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u@pkh.me> wrote: > > From: Clément Bœsch <cboesch@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. > Can the av_log call be moved under a local variable for the condition to > after the lock is released?
Hi, On Fri, Jan 13, 2017 at 9:39 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 13 Jan 2017 09:07:48 -0500 > "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > Hi, > > > > On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u@pkh.me> wrote: > > > > From: Clément Bœsch <cboesch@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(..); Ronald
On Fri, 13 Jan 2017 12:19:14 -0500 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Jan 13, 2017 at 9:39 AM, wm4 <nfxjfg@googlemail.com> wrote: > > > On Fri, 13 Jan 2017 09:07:48 -0500 > > "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > > > Hi, > > > > > > On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u@pkh.me> wrote: > > > > > > From: Clément Bœsch <cboesch@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?
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@gmail.com> wrote: > > > Hi, > > > > On Fri, Jan 13, 2017 at 9:39 AM, wm4 <nfxjfg@googlemail.com> wrote: > > > > > On Fri, 13 Jan 2017 09:07:48 -0500 > > > "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > > > > > Hi, > > > > > > > > On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u@pkh.me> wrote: > > > > > > > > From: Clément Bœsch <cboesch@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
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);
Hi, On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u@pkh.me> wrote: From: Clément Bœsch <cboesch@gopro.com> --- libavcodec/pthread_frame.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) This has a problem in that we're potentially calling an I/O function that potentially directly enters application space with a lock held. Can the av_log call be moved under a local variable for the condition to after the lock is released? Ronald