| Message ID | tencent_A09A099F80AB014EB3706F2D0A9AF640CB08@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/vvc: Use static const for function table | 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 08.07.2024 09:43, Zhao Zhili wrote: > From: Zhao Zhili <zhilizhao@tencent.com> > > av_executor_execute run the task directly when thread is disabled. > The task can schedule a new task by call av_executor_execute. This > forms an implicit recursive call. This patch removed the recursive > call. > --- > libavutil/executor.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavutil/executor.c b/libavutil/executor.c > index 89058fab2f..c145c51be9 100644 > --- a/libavutil/executor.c > +++ b/libavutil/executor.c > @@ -58,6 +58,7 @@ struct AVExecutor { > int die; > > AVTask *tasks; > + int stack_depth; > }; > > static AVTask* remove_task(AVTask **prev, AVTask *t) > @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t) > } > > if (!e->thread_count || !HAVE_THREADS) { > + if (e->stack_depth > 0) > + return; > + e->stack_depth++; > // We are running in a single-threaded environment, so we must handle all tasks ourselves > while (run_one_task(e, e->local_contexts)) > /* nothing */; > + e->stack_depth--; Won't this put the above line into the "nothing" while-loop? Is that intended? If so, the indentation should be adjusted accordingly. If not, the while-loop should gain empty {}. > } > }
> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> wrote: > > On 08.07.2024 09:43, Zhao Zhili wrote: >> From: Zhao Zhili <zhilizhao@tencent.com> >> av_executor_execute run the task directly when thread is disabled. >> The task can schedule a new task by call av_executor_execute. This >> forms an implicit recursive call. This patch removed the recursive >> call. >> --- >> libavutil/executor.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> diff --git a/libavutil/executor.c b/libavutil/executor.c >> index 89058fab2f..c145c51be9 100644 >> --- a/libavutil/executor.c >> +++ b/libavutil/executor.c >> @@ -58,6 +58,7 @@ struct AVExecutor { >> int die; >> AVTask *tasks; >> + int stack_depth; >> }; >> static AVTask* remove_task(AVTask **prev, AVTask *t) >> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t) >> } >> if (!e->thread_count || !HAVE_THREADS) { >> + if (e->stack_depth > 0) >> + return; >> + e->stack_depth++; >> // We are running in a single-threaded environment, so we must handle all tasks ourselves >> while (run_one_task(e, e->local_contexts)) >> /* nothing */; >> + e->stack_depth--; > > Won't this put the above line into the "nothing" while-loop? > Is that intended? If so, the indentation should be adjusted accordingly. > If not, the while-loop should gain empty {}. The comment specify it’s a while loop with empty body. Maybe it’s not obvious in the email client. > >> } >> } > _______________________________________________ > 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 08.07.2024 17:32, Zhao Zhili wrote: > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> wrote: >> >> On 08.07.2024 09:43, Zhao Zhili wrote: >>> From: Zhao Zhili <zhilizhao@tencent.com> >>> av_executor_execute run the task directly when thread is disabled. >>> The task can schedule a new task by call av_executor_execute. This >>> forms an implicit recursive call. This patch removed the recursive >>> call. >>> --- >>> libavutil/executor.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> diff --git a/libavutil/executor.c b/libavutil/executor.c >>> index 89058fab2f..c145c51be9 100644 >>> --- a/libavutil/executor.c >>> +++ b/libavutil/executor.c >>> @@ -58,6 +58,7 @@ struct AVExecutor { >>> int die; >>> AVTask *tasks; >>> + int stack_depth; >>> }; >>> static AVTask* remove_task(AVTask **prev, AVTask *t) >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t) >>> } >>> if (!e->thread_count || !HAVE_THREADS) { >>> + if (e->stack_depth > 0) >>> + return; >>> + e->stack_depth++; >>> // We are running in a single-threaded environment, so we must handle all tasks ourselves >>> while (run_one_task(e, e->local_contexts)) >>> /* nothing */; >>> + e->stack_depth--; >> >> Won't this put the above line into the "nothing" while-loop? >> Is that intended? If so, the indentation should be adjusted accordingly. >> If not, the while-loop should gain empty {}. > > The comment specify it’s a while loop with empty body. Maybe it’s not obvious > in the email client. Oh, there is a ; behind the comment. Completely missed that. Can't say I like it, but yeah, that works.
On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org> wrote: > On 08.07.2024 17:32, Zhao Zhili wrote: > > > > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> > wrote: > >> > >> On 08.07.2024 09:43, Zhao Zhili wrote: > >>> From: Zhao Zhili <zhilizhao@tencent.com> > >>> av_executor_execute run the task directly when thread is disabled. > >>> The task can schedule a new task by call av_executor_execute. This > >>> forms an implicit recursive call. This patch removed the recursive > >>> call. > >>> --- > >>> libavutil/executor.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c > >>> index 89058fab2f..c145c51be9 100644 > >>> --- a/libavutil/executor.c > >>> +++ b/libavutil/executor.c > >>> @@ -58,6 +58,7 @@ struct AVExecutor { > >>> int die; > >>> AVTask *tasks; > >>> + int stack_depth; > >>> }; > >>> static AVTask* remove_task(AVTask **prev, AVTask *t) > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t) > >>> } > >>> if (!e->thread_count || !HAVE_THREADS) { > >>> + if (e->stack_depth > 0) > >>> + return; > >>> + e->stack_depth++; > >>> // We are running in a single-threaded environment, so we > must handle all tasks ourselves > >>> while (run_one_task(e, e->local_contexts)) > >>> /* nothing */; > >>> + e->stack_depth--; > >> > >> Won't this put the above line into the "nothing" while-loop? > >> Is that intended? If so, the indentation should be adjusted accordingly. > >> If not, the while-loop should gain empty {}. > > > > The comment specify it’s a while loop with empty body. Maybe it’s not > obvious > > in the email client. > > Oh, there is a ; behind the comment. > Completely missed that. Can't say I like it, but yeah, that works. > Google suggests using "while (cond) continue;": https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching . However, it's not better than using /* nothing */. > _______________________________________________ > 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". >
Quoting Nuo Mi (2024-07-09 13:36:09) > On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org> > wrote: > > > On 08.07.2024 17:32, Zhao Zhili wrote: > > > > > > > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> > > wrote: > > >> > > >> On 08.07.2024 09:43, Zhao Zhili wrote: > > >>> From: Zhao Zhili <zhilizhao@tencent.com> > > >>> av_executor_execute run the task directly when thread is disabled. > > >>> The task can schedule a new task by call av_executor_execute. This > > >>> forms an implicit recursive call. This patch removed the recursive > > >>> call. > > >>> --- > > >>> libavutil/executor.c | 5 +++++ > > >>> 1 file changed, 5 insertions(+) > > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c > > >>> index 89058fab2f..c145c51be9 100644 > > >>> --- a/libavutil/executor.c > > >>> +++ b/libavutil/executor.c > > >>> @@ -58,6 +58,7 @@ struct AVExecutor { > > >>> int die; > > >>> AVTask *tasks; > > >>> + int stack_depth; > > >>> }; > > >>> static AVTask* remove_task(AVTask **prev, AVTask *t) > > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t) > > >>> } > > >>> if (!e->thread_count || !HAVE_THREADS) { > > >>> + if (e->stack_depth > 0) > > >>> + return; > > >>> + e->stack_depth++; > > >>> // We are running in a single-threaded environment, so we > > must handle all tasks ourselves > > >>> while (run_one_task(e, e->local_contexts)) > > >>> /* nothing */; > > >>> + e->stack_depth--; > > >> > > >> Won't this put the above line into the "nothing" while-loop? > > >> Is that intended? If so, the indentation should be adjusted accordingly. > > >> If not, the while-loop should gain empty {}. > > > > > > The comment specify it’s a while loop with empty body. Maybe it’s not > > obvious > > > in the email client. > > > > Oh, there is a ; behind the comment. > > Completely missed that. Can't say I like it, but yeah, that works. > > > Google suggests using "while (cond) continue;": > https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching > . > However, it's not better than using /* nothing */. continue gets hilit by your editor. Also, I'd put it on a separate line for extra visibility. And bikesheds should be black with red stripes.
On Wed, Jul 10, 2024 at 5:14 PM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Nuo Mi (2024-07-09 13:36:09) > > On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org> > > wrote: > > > > > On 08.07.2024 17:32, Zhao Zhili wrote: > > > > > > > > > > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> > > > wrote: > > > >> > > > >> On 08.07.2024 09:43, Zhao Zhili wrote: > > > >>> From: Zhao Zhili <zhilizhao@tencent.com> > > > >>> av_executor_execute run the task directly when thread is disabled. > > > >>> The task can schedule a new task by call av_executor_execute. This > > > >>> forms an implicit recursive call. This patch removed the recursive > > > >>> call. > > > >>> --- > > > >>> libavutil/executor.c | 5 +++++ > > > >>> 1 file changed, 5 insertions(+) > > > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c > > > >>> index 89058fab2f..c145c51be9 100644 > > > >>> --- a/libavutil/executor.c > > > >>> +++ b/libavutil/executor.c > > > >>> @@ -58,6 +58,7 @@ struct AVExecutor { > > > >>> int die; > > > >>> AVTask *tasks; > > > >>> + int stack_depth; > > > >>> }; > > > >>> static AVTask* remove_task(AVTask **prev, AVTask *t) > > > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, > AVTask *t) > > > >>> } > > > >>> if (!e->thread_count || !HAVE_THREADS) { > > > >>> + if (e->stack_depth > 0) > > > >>> + return; > > > >>> + e->stack_depth++; > > > >>> // We are running in a single-threaded environment, so we > > > must handle all tasks ourselves > > > >>> while (run_one_task(e, e->local_contexts)) > > > >>> /* nothing */; > > > >>> + e->stack_depth--; > > > >> > > > >> Won't this put the above line into the "nothing" while-loop? > > > >> Is that intended? If so, the indentation should be adjusted > accordingly. > > > >> If not, the while-loop should gain empty {}. > > > > > > > > The comment specify it’s a while loop with empty body. Maybe it’s not > > > obvious > > > > in the email client. > > > > > > Oh, there is a ; behind the comment. > > > Completely missed that. Can't say I like it, but yeah, that works. > > > > > Google suggests using "while (cond) continue;": > > > https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching > > . > > However, it's not better than using /* nothing */. > > continue gets hilit by your editor. > > Also, I'd put it on a separate line for extra visibility. > > And bikesheds should be black with red stripes. > Editors will highlight the comment with green too, which will help protect our eyes. > > -- > Anton Khirnov > _______________________________________________ > 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 Thu, Jul 11, 2024 at 8:21 PM Nuo Mi <nuomi2021@gmail.com> wrote: > > > On Wed, Jul 10, 2024 at 5:14 PM Anton Khirnov <anton@khirnov.net> wrote: > >> Quoting Nuo Mi (2024-07-09 13:36:09) >> > On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org >> > >> > wrote: >> > >> > > On 08.07.2024 17:32, Zhao Zhili wrote: >> > > > >> > > > >> > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org >> > >> > > wrote: >> > > >> >> > > >> On 08.07.2024 09:43, Zhao Zhili wrote: >> > > >>> From: Zhao Zhili <zhilizhao@tencent.com> >> > > >>> av_executor_execute run the task directly when thread is disabled. >> > > >>> The task can schedule a new task by call av_executor_execute. This >> > > >>> forms an implicit recursive call. This patch removed the recursive >> > > >>> call. >> > > >>> --- >> > > >>> libavutil/executor.c | 5 +++++ >> > > >>> 1 file changed, 5 insertions(+) >> > > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c >> > > >>> index 89058fab2f..c145c51be9 100644 >> > > >>> --- a/libavutil/executor.c >> > > >>> +++ b/libavutil/executor.c >> > > >>> @@ -58,6 +58,7 @@ struct AVExecutor { >> > > >>> int die; >> > > >>> AVTask *tasks; >> > > >>> + int stack_depth; >> > > >>> }; >> > > >>> static AVTask* remove_task(AVTask **prev, AVTask *t) >> > > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, >> AVTask *t) >> > > >>> } >> > > >>> if (!e->thread_count || !HAVE_THREADS) { >> > > >>> + if (e->stack_depth > 0) >> > > >>> + return; >> > > >>> + e->stack_depth++; >> > > >>> // We are running in a single-threaded environment, so >> we >> > > must handle all tasks ourselves >> > > >>> while (run_one_task(e, e->local_contexts)) >> > > >>> /* nothing */; >> > > >>> + e->stack_depth--; >> > > >> >> > > >> Won't this put the above line into the "nothing" while-loop? >> > > >> Is that intended? If so, the indentation should be adjusted >> accordingly. >> > > >> If not, the while-loop should gain empty {}. >> > > > >> > > > The comment specify it’s a while loop with empty body. Maybe it’s >> not >> > > obvious >> > > > in the email client. >> > > >> > > Oh, there is a ; behind the comment. >> > > Completely missed that. Can't say I like it, but yeah, that works. >> > > >> > Google suggests using "while (cond) continue;": >> > >> https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching >> > . >> > However, it's not better than using /* nothing */. >> >> continue gets hilit by your editor. >> >> Also, I'd put it on a separate line for extra visibility. >> >> And bikesheds should be black with red stripes. >> > Editors will highlight the comment with green too, which will help protect > our eyes. > Applied. Thank you, for the patch and review. >> -- >> Anton Khirnov >> _______________________________________________ >> 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 89058fab2f..c145c51be9 100644 --- a/libavutil/executor.c +++ b/libavutil/executor.c @@ -58,6 +58,7 @@ struct AVExecutor { int die; AVTask *tasks; + int stack_depth; }; static AVTask* remove_task(AVTask **prev, AVTask *t) @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t) } if (!e->thread_count || !HAVE_THREADS) { + if (e->stack_depth > 0) + return; + e->stack_depth++; // We are running in a single-threaded environment, so we must handle all tasks ourselves while (run_one_task(e, e->local_contexts)) /* nothing */; + e->stack_depth--; } }
From: Zhao Zhili <zhilizhao@tencent.com> av_executor_execute run the task directly when thread is disabled. The task can schedule a new task by call av_executor_execute. This forms an implicit recursive call. This patch removed the recursive call. --- libavutil/executor.c | 5 +++++ 1 file changed, 5 insertions(+)