* [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