Re: [FFmpeg-devel] [PATCH 2/2] avutil/executor: Fix stack overflow due to recursive call

[Nuo Mi <nuomi2021@gmail.com>]

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