* [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex @ 2024-06-30 10:45 Zhao Zhili 2024-07-01 13:14 ` Nuo Mi 0 siblings, 1 reply; 5+ messages in thread From: Zhao Zhili @ 2024-06-30 10:45 UTC (permalink / raw) To: ffmpeg-devel; +Cc: Zhao Zhili 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(-) 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". ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex 2024-06-30 10:45 [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex Zhao Zhili @ 2024-07-01 13:14 ` Nuo Mi 2024-07-01 13:59 ` Zhao Zhili 0 siblings, 1 reply; 5+ messages in thread From: Nuo Mi @ 2024-07-01 13:14 UTC (permalink / raw) To: FFmpeg development discussions and patches; +Cc: Zhao Zhili 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". > _______________________________________________ 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". ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex 2024-07-01 13:14 ` Nuo Mi @ 2024-07-01 13:59 ` Zhao Zhili 2024-07-01 14:13 ` Nuo Mi 0 siblings, 1 reply; 5+ messages in thread From: Zhao Zhili @ 2024-07-01 13:59 UTC (permalink / raw) To: FFmpeg development discussions and patches > 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". _______________________________________________ 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". ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex 2024-07-01 13:59 ` Zhao Zhili @ 2024-07-01 14:13 ` Nuo Mi 2024-07-11 12:33 ` Nuo Mi 0 siblings, 1 reply; 5+ messages in thread From: Nuo Mi @ 2024-07-01 14:13 UTC (permalink / raw) To: FFmpeg development discussions and patches 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". > _______________________________________________ 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". ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex 2024-07-01 14:13 ` Nuo Mi @ 2024-07-11 12:33 ` Nuo Mi 0 siblings, 0 replies; 5+ messages in thread From: Nuo Mi @ 2024-07-11 12:33 UTC (permalink / raw) To: FFmpeg development discussions and patches 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". >> > _______________________________________________ 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". ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-07-11 12:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-30 10:45 [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex Zhao Zhili 2024-07-01 13:14 ` Nuo Mi 2024-07-01 13:59 ` Zhao Zhili 2024-07-01 14:13 ` Nuo Mi 2024-07-11 12:33 ` Nuo Mi
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git