| Message ID | tencent_E5EA8F53255C5CCB8766159093E6E527F106@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avutil/executor: Fix missing check before using mutex | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Sun, Jun 30, 2024 at 6:45 PM Zhao Zhili <quinkblack@foxmail.com> wrote: > From: Zhao Zhili <zhilizhao@tencent.com> > > --- > The code can be simplified by always creating mutex/cond. I'm not sure > it worth the overhead. Please note !HAVE_THREADS don't have the same > problem since it has mock implementation of ff_mutex_lock/unlock. > Hi Zhili, Thank you for the patch. Do we really need this? The lock/unlock/signal functions will return an error if the lock and condition variables are not initialized. > > libavutil/executor.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavutil/executor.c b/libavutil/executor.c > index fb20104b58..89058fab2f 100644 > --- a/libavutil/executor.c > +++ b/libavutil/executor.c > @@ -194,14 +194,17 @@ void av_executor_execute(AVExecutor *e, AVTask *t) > AVTaskCallbacks *cb = &e->cb; > AVTask **prev; > > - ff_mutex_lock(&e->lock); > + if (e->thread_count) > + ff_mutex_lock(&e->lock); > if (t) { > for (prev = &e->tasks; *prev && cb->priority_higher(*prev, t); > prev = &(*prev)->next) > /* nothing */; > add_task(prev, t); > } > - ff_cond_signal(&e->cond); > - ff_mutex_unlock(&e->lock); > + if (e->thread_count) { > + ff_cond_signal(&e->cond); > + ff_mutex_unlock(&e->lock); > + } > > if (!e->thread_count || !HAVE_THREADS) { > // We are running in a single-threaded environment, so we must > handle all tasks ourselves > -- > 2.42.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
> On Jul 1, 2024, at 21:14, Nuo Mi <nuomi2021@gmail.com> wrote: > > On Sun, Jun 30, 2024 at 6:45 PM Zhao Zhili <quinkblack@foxmail.com> wrote: > >> From: Zhao Zhili <zhilizhao@tencent.com> >> >> --- >> The code can be simplified by always creating mutex/cond. I'm not sure >> it worth the overhead. Please note !HAVE_THREADS don't have the same >> problem since it has mock implementation of ff_mutex_lock/unlock. >> > Hi Zhili, > Thank you for the patch. > Do we really need this? The lock/unlock/signal functions will return an > error if the lock and condition variables are not initialized. We have strict check in libavutil/thread.h, e.g., static inline int strict_pthread_mutex_lock(pthread_mutex_t *mutex) { ASSERT_PTHREAD(pthread_mutex_lock, mutex); } The strict check is enabled when configure --assert-level=2. > >> >> libavutil/executor.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/libavutil/executor.c b/libavutil/executor.c >> index fb20104b58..89058fab2f 100644 >> --- a/libavutil/executor.c >> +++ b/libavutil/executor.c >> @@ -194,14 +194,17 @@ void av_executor_execute(AVExecutor *e, AVTask *t) >> AVTaskCallbacks *cb = &e->cb; >> AVTask **prev; >> >> - ff_mutex_lock(&e->lock); >> + if (e->thread_count) >> + ff_mutex_lock(&e->lock); >> if (t) { >> for (prev = &e->tasks; *prev && cb->priority_higher(*prev, t); >> prev = &(*prev)->next) >> /* nothing */; >> add_task(prev, t); >> } >> - ff_cond_signal(&e->cond); >> - ff_mutex_unlock(&e->lock); >> + if (e->thread_count) { >> + ff_cond_signal(&e->cond); >> + ff_mutex_unlock(&e->lock); >> + } >> >> if (!e->thread_count || !HAVE_THREADS) { >> // We are running in a single-threaded environment, so we must >> handle all tasks ourselves >> -- >> 2.42.0 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Mon, Jul 1, 2024 at 10:00 PM Zhao Zhili <quinkblack@foxmail.com> wrote: > > > > On Jul 1, 2024, at 21:14, Nuo Mi <nuomi2021@gmail.com> wrote: > > > > On Sun, Jun 30, 2024 at 6:45 PM Zhao Zhili <quinkblack@foxmail.com> > wrote: > > > >> From: Zhao Zhili <zhilizhao@tencent.com> > >> > >> --- > >> The code can be simplified by always creating mutex/cond. I'm not sure > >> it worth the overhead. Please note !HAVE_THREADS don't have the same > >> problem since it has mock implementation of ff_mutex_lock/unlock. > >> > > Hi Zhili, > > Thank you for the patch. > > Do we really need this? The lock/unlock/signal functions will return an > > error if the lock and condition variables are not initialized. > > We have strict check in libavutil/thread.h, e.g., > > static inline int strict_pthread_mutex_lock(pthread_mutex_t *mutex) > { > ASSERT_PTHREAD(pthread_mutex_lock, mutex); > } > > The strict check is enabled when configure --assert-level=2. > LGTM. Thank you. > > > > >> > >> libavutil/executor.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/libavutil/executor.c b/libavutil/executor.c > >> index fb20104b58..89058fab2f 100644 > >> --- a/libavutil/executor.c > >> +++ b/libavutil/executor.c > >> @@ -194,14 +194,17 @@ void av_executor_execute(AVExecutor *e, AVTask *t) > >> AVTaskCallbacks *cb = &e->cb; > >> AVTask **prev; > >> > >> - ff_mutex_lock(&e->lock); > >> + if (e->thread_count) > >> + ff_mutex_lock(&e->lock); > >> if (t) { > >> for (prev = &e->tasks; *prev && cb->priority_higher(*prev, t); > >> prev = &(*prev)->next) > >> /* nothing */; > >> add_task(prev, t); > >> } > >> - ff_cond_signal(&e->cond); > >> - ff_mutex_unlock(&e->lock); > >> + if (e->thread_count) { > >> + ff_cond_signal(&e->cond); > >> + ff_mutex_unlock(&e->lock); > >> + } > >> > >> if (!e->thread_count || !HAVE_THREADS) { > >> // We are running in a single-threaded environment, so we must > >> handle all tasks ourselves > >> -- > >> 2.42.0 > >> > >> _______________________________________________ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > >> To unsubscribe, visit link above, or email > >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Mon, Jul 1, 2024 at 10:13 PM Nuo Mi <nuomi2021@gmail.com> wrote: > > > On Mon, Jul 1, 2024 at 10:00 PM Zhao Zhili <quinkblack@foxmail.com> wrote: > >> >> >> > On Jul 1, 2024, at 21:14, Nuo Mi <nuomi2021@gmail.com> wrote: >> > >> > On Sun, Jun 30, 2024 at 6:45 PM Zhao Zhili <quinkblack@foxmail.com> >> wrote: >> > >> >> From: Zhao Zhili <zhilizhao@tencent.com> >> >> >> >> --- >> >> The code can be simplified by always creating mutex/cond. I'm not sure >> >> it worth the overhead. Please note !HAVE_THREADS don't have the same >> >> problem since it has mock implementation of ff_mutex_lock/unlock. >> >> >> > Hi Zhili, >> > Thank you for the patch. >> > Do we really need this? The lock/unlock/signal functions will return an >> > error if the lock and condition variables are not initialized. >> >> We have strict check in libavutil/thread.h, e.g., >> >> static inline int strict_pthread_mutex_lock(pthread_mutex_t *mutex) >> { >> ASSERT_PTHREAD(pthread_mutex_lock, mutex); >> } >> >> The strict check is enabled when configure --assert-level=2. >> > LGTM. > Thank you. > merged. thx. > >> > >> >> >> >> libavutil/executor.c | 9 ++++++--- >> >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/libavutil/executor.c b/libavutil/executor.c >> >> index fb20104b58..89058fab2f 100644 >> >> --- a/libavutil/executor.c >> >> +++ b/libavutil/executor.c >> >> @@ -194,14 +194,17 @@ void av_executor_execute(AVExecutor *e, AVTask >> *t) >> >> AVTaskCallbacks *cb = &e->cb; >> >> AVTask **prev; >> >> >> >> - ff_mutex_lock(&e->lock); >> >> + if (e->thread_count) >> >> + ff_mutex_lock(&e->lock); >> >> if (t) { >> >> for (prev = &e->tasks; *prev && cb->priority_higher(*prev, t); >> >> prev = &(*prev)->next) >> >> /* nothing */; >> >> add_task(prev, t); >> >> } >> >> - ff_cond_signal(&e->cond); >> >> - ff_mutex_unlock(&e->lock); >> >> + if (e->thread_count) { >> >> + ff_cond_signal(&e->cond); >> >> + ff_mutex_unlock(&e->lock); >> >> + } >> >> >> >> if (!e->thread_count || !HAVE_THREADS) { >> >> // We are running in a single-threaded environment, so we must >> >> handle all tasks ourselves >> >> -- >> >> 2.42.0 >> >> >> >> _______________________________________________ >> >> ffmpeg-devel mailing list >> >> ffmpeg-devel@ffmpeg.org >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> >> To unsubscribe, visit link above, or email >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >> >> > _______________________________________________ >> > ffmpeg-devel mailing list >> > ffmpeg-devel@ffmpeg.org >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > >> > To unsubscribe, visit link above, or email >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >
diff --git a/libavutil/executor.c b/libavutil/executor.c index fb20104b58..89058fab2f 100644 --- a/libavutil/executor.c +++ b/libavutil/executor.c @@ -194,14 +194,17 @@ void av_executor_execute(AVExecutor *e, AVTask *t) AVTaskCallbacks *cb = &e->cb; AVTask **prev; - ff_mutex_lock(&e->lock); + if (e->thread_count) + ff_mutex_lock(&e->lock); if (t) { for (prev = &e->tasks; *prev && cb->priority_higher(*prev, t); prev = &(*prev)->next) /* nothing */; add_task(prev, t); } - ff_cond_signal(&e->cond); - ff_mutex_unlock(&e->lock); + if (e->thread_count) { + ff_cond_signal(&e->cond); + ff_mutex_unlock(&e->lock); + } if (!e->thread_count || !HAVE_THREADS) { // We are running in a single-threaded environment, so we must handle all tasks ourselves
From: Zhao Zhili <zhilizhao@tencent.com> --- The code can be simplified by always creating mutex/cond. I'm not sure it worth the overhead. Please note !HAVE_THREADS don't have the same problem since it has mock implementation of ff_mutex_lock/unlock. libavutil/executor.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)