diff mbox series

[FFmpeg-devel,2/2] avutil/slicethread: Add a maximum constraint of 16 slice threads

Message ID 1636115536-13580-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avfilter/af_atilt: use ff_filter_execute() | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Lance Wang Nov. 5, 2021, 12:32 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/slicethread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gyan Doshi Nov. 5, 2021, 12:56 p.m. UTC | #1 
On 2021-11-05 06:02 pm, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>   libavutil/slicethread.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
> index 867ce32..7b3a106 100644
> --- a/libavutil/slicethread.c
> +++ b/libavutil/slicethread.c
> @@ -104,7 +104,7 @@ int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
>       if (!nb_threads) {
>           int nb_cpus = av_cpu_count();
>           if (nb_cpus > 1)
> -            nb_threads = nb_cpus + 1;
> +            nb_threads = FFMIN(nb_cpus + 1, 16);

Do you see diminishing returns with nb_threads > 16? Or slower filter 
throughput?

Regards,
Gyan
Paul B Mahol Nov. 5, 2021, 7:22 p.m. UTC | #2 
NACK
Michael Niedermayer Nov. 5, 2021, 9 p.m. UTC | #3 
On Fri, Nov 05, 2021 at 08:32:16PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/slicethread.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
> index 867ce32..7b3a106 100644
> --- a/libavutil/slicethread.c
> +++ b/libavutil/slicethread.c
> @@ -104,7 +104,7 @@ int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
>      if (!nb_threads) {
>          int nb_cpus = av_cpu_count();
>          if (nb_cpus > 1)
> -            nb_threads = nb_cpus + 1;
> +            nb_threads = FFMIN(nb_cpus + 1, 16);

why should the threads be limited ?
why should they be limited at 16 for everyone ?
one mighht be a 2 core cpu one might have 200 cores

i think the commit message should be more verbose describing what the
problem is that this is fixing

thx

[...]
Lance Wang Nov. 6, 2021, 5:26 a.m. UTC | #4 
On Fri, Nov 05, 2021 at 10:00:41PM +0100, Michael Niedermayer wrote:
> On Fri, Nov 05, 2021 at 08:32:16PM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavutil/slicethread.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
> > index 867ce32..7b3a106 100644
> > --- a/libavutil/slicethread.c
> > +++ b/libavutil/slicethread.c
> > @@ -104,7 +104,7 @@ int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
> >      if (!nb_threads) {
> >          int nb_cpus = av_cpu_count();
> >          if (nb_cpus > 1)
> > -            nb_threads = nb_cpus + 1;
> > +            nb_threads = FFMIN(nb_cpus + 1, 16);
> 
> why should the threads be limited ?
> why should they be limited at 16 for everyone ?
> one mighht be a 2 core cpu one might have 200 cores

When using movie filter, it use ff_filter_get_nb_threads(ctx), but we can't force the thread
number by option, so it's auto thread mode. When testing one a 4 core cpu(about 200 cores), 
the auto thread mode will use all cores for video decode, it'll drop the performance very much.
If most of user prefer to use all cores, I'll try to limit the number in movie filter only.

For a 4 socket numa system, I don't think it's preferable configure to use all cpu cores
if it's auto thread(nb_threads = 0).


> 
> i think the commit message should be more verbose describing what the
> problem is that this is fixing
> 
> thx
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Homeopathy is like voting while filling the ballot out with transparent ink.
> Sometimes the outcome one wanted occurs. Rarely its worse than filling out
> a ballot properly.



> _______________________________________________
> 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".
Hendrik Leppkes Nov. 6, 2021, 7:42 a.m. UTC | #5 
On Sat, Nov 6, 2021 at 6:26 AM <lance.lmwang@gmail.com> wrote:
>
> On Fri, Nov 05, 2021 at 10:00:41PM +0100, Michael Niedermayer wrote:
> > On Fri, Nov 05, 2021 at 08:32:16PM +0800, lance.lmwang@gmail.com wrote:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavutil/slicethread.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
> > > index 867ce32..7b3a106 100644
> > > --- a/libavutil/slicethread.c
> > > +++ b/libavutil/slicethread.c
> > > @@ -104,7 +104,7 @@ int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
> > >      if (!nb_threads) {
> > >          int nb_cpus = av_cpu_count();
> > >          if (nb_cpus > 1)
> > > -            nb_threads = nb_cpus + 1;
> > > +            nb_threads = FFMIN(nb_cpus + 1, 16);
> >
> > why should the threads be limited ?
> > why should they be limited at 16 for everyone ?
> > one mighht be a 2 core cpu one might have 200 cores
>
> When using movie filter, it use ff_filter_get_nb_threads(ctx), but we can't force the thread
> number by option, so it's auto thread mode. When testing one a 4 core cpu(about 200 cores),
> the auto thread mode will use all cores for video decode, it'll drop the performance very much.
> If most of user prefer to use all cores, I'll try to limit the number in movie filter only.
>
> For a 4 socket numa system, I don't think it's preferable configure to use all cpu cores
> if it's auto thread(nb_threads = 0).
>
>

The answer to that issue should be to make it configurable, and not
just introduce a hard limit in a utility function in avutil.

- Hendrik
Lance Wang Nov. 6, 2021, 12:54 p.m. UTC | #6 
On Sat, Nov 06, 2021 at 08:42:38AM +0100, Hendrik Leppkes wrote:
> On Sat, Nov 6, 2021 at 6:26 AM <lance.lmwang@gmail.com> wrote:
> >
> > On Fri, Nov 05, 2021 at 10:00:41PM +0100, Michael Niedermayer wrote:
> > > On Fri, Nov 05, 2021 at 08:32:16PM +0800, lance.lmwang@gmail.com wrote:
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > >
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  libavutil/slicethread.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
> > > > index 867ce32..7b3a106 100644
> > > > --- a/libavutil/slicethread.c
> > > > +++ b/libavutil/slicethread.c
> > > > @@ -104,7 +104,7 @@ int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
> > > >      if (!nb_threads) {
> > > >          int nb_cpus = av_cpu_count();
> > > >          if (nb_cpus > 1)
> > > > -            nb_threads = nb_cpus + 1;
> > > > +            nb_threads = FFMIN(nb_cpus + 1, 16);
> > >
> > > why should the threads be limited ?
> > > why should they be limited at 16 for everyone ?
> > > one mighht be a 2 core cpu one might have 200 cores
> >
> > When using movie filter, it use ff_filter_get_nb_threads(ctx), but we can't force the thread
> > number by option, so it's auto thread mode. When testing one a 4 core cpu(about 200 cores),
> > the auto thread mode will use all cores for video decode, it'll drop the performance very much.
> > If most of user prefer to use all cores, I'll try to limit the number in movie filter only.
> >
> > For a 4 socket numa system, I don't think it's preferable configure to use all cpu cores
> > if it's auto thread(nb_threads = 0).
> >
> >
> 
> The answer to that issue should be to make it configurable, and not
> just introduce a hard limit in a utility function in avutil.

OK, I'll add a thread option for src_movie filter to make the decode thread configurable.

> 
> - Hendrik
> _______________________________________________
> 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 mbox series

Patch

diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
index 867ce32..7b3a106 100644
--- a/libavutil/slicethread.c
+++ b/libavutil/slicethread.c
@@ -104,7 +104,7 @@  int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
     if (!nb_threads) {
         int nb_cpus = av_cpu_count();
         if (nb_cpus > 1)
-            nb_threads = nb_cpus + 1;
+            nb_threads = FFMIN(nb_cpus + 1, 16);
         else
             nb_threads = 1;
     }