Re: [FFmpeg-devel] [PATCH] avutil/executor: Fix missing check before using mutex

[Nuo Mi <nuomi2021@gmail.com>]

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".