[FFmpeg-devel] lavc/libaomenc: Add a maximum constraint of 64 encoder threads.

Submitted by Jun Zhao on Nov. 27, 2018, 9:18 a.m.

Details

Message ID 1543310306-2050-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao Nov. 27, 2018, 9:18 a.m.
fixed the error in Intel(R) Xeon(R) Gold 6152 CPU like:
[libaom-av1 @ 0x469f340] Failed to initialize encoder: Invalid parameter
[libaom-av1 @ 0x469f340]   Additional information: g_threads out of range [..MAX_NUM_THREADS]

Signed-off-by: Jun Zhao <mypopydev@gmail.com>
---
 libavcodec/libaomenc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Carl Eugen Hoyos Nov. 27, 2018, 8:46 p.m.
2018-11-27 10:18 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> fixed the error in Intel(R) Xeon(R) Gold 6152 CPU like:
> [libaom-av1 @ 0x469f340] Failed to initialize encoder: Invalid parameter
> [libaom-av1 @ 0x469f340]   Additional information: g_threads out of range
> [..MAX_NUM_THREADS]
>
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  libavcodec/libaomenc.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index cb31c55..ccb0cf9 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -504,7 +504,8 @@ static av_cold int aom_init(AVCodecContext *avctx,
>      enccfg.g_h            = avctx->height;
>      enccfg.g_timebase.num = avctx->time_base.num;
>      enccfg.g_timebase.den = avctx->time_base.den;
> -    enccfg.g_threads      = avctx->thread_count ? avctx->thread_count :
> av_cpu_count();
> +    enccfg.g_threads      =
> +	FFMIN(avctx->thread_count ? avctx->thread_count : av_cpu_count(), 64);

Is there a limitation in libaom's api that requires users to never set
g_threads >64 that we missed so far?
Or is there a bug in libaom that you would like to fix in FFmpeg?

Please explain, Carl Eugen
mypopy@gmail.com Nov. 28, 2018, 12:54 a.m.
On Wed, Nov 28, 2018 at 4:47 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-11-27 10:18 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> > fixed the error in Intel(R) Xeon(R) Gold 6152 CPU like:
> > [libaom-av1 @ 0x469f340] Failed to initialize encoder: Invalid parameter
> > [libaom-av1 @ 0x469f340]   Additional information: g_threads out of range
> > [..MAX_NUM_THREADS]
> >
> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> > ---
> >  libavcodec/libaomenc.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> > index cb31c55..ccb0cf9 100644
> > --- a/libavcodec/libaomenc.c
> > +++ b/libavcodec/libaomenc.c
> > @@ -504,7 +504,8 @@ static av_cold int aom_init(AVCodecContext *avctx,
> >      enccfg.g_h            = avctx->height;
> >      enccfg.g_timebase.num = avctx->time_base.num;
> >      enccfg.g_timebase.den = avctx->time_base.den;
> > -    enccfg.g_threads      = avctx->thread_count ? avctx->thread_count :
> > av_cpu_count();
> > +    enccfg.g_threads      =
> > +     FFMIN(avctx->thread_count ? avctx->thread_count : av_cpu_count(),
> 64);
>
> Is there a limitation in libaom's api that requires users to never set
> g_threads >64 that we missed so far?
> Or is there a bug in libaom that you would like to fix in FFmpeg?
>
> Please explain, Carl Eugen
>
> I  belive this is a libaom's limitation and we missed the check in FFmpeg
part,
libvpx have the same issue and the commit d6b1248fc try to fix the same
issue.
Carl Eugen Hoyos Nov. 28, 2018, 1:41 a.m.
2018-11-28 1:54 GMT+01:00, mypopy@gmail.com <mypopy@gmail.com>:
> On Wed, Nov 28, 2018 at 4:47 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2018-11-27 10:18 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
>> > fixed the error in Intel(R) Xeon(R) Gold 6152 CPU like:
>> > [libaom-av1 @ 0x469f340] Failed to initialize encoder: Invalid parameter
>> > [libaom-av1 @ 0x469f340]   Additional information: g_threads out of
>> > range
>> > [..MAX_NUM_THREADS]
>> >
>> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>> > ---
>> >  libavcodec/libaomenc.c |    3 ++-
>> >  1 files changed, 2 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
>> > index cb31c55..ccb0cf9 100644
>> > --- a/libavcodec/libaomenc.c
>> > +++ b/libavcodec/libaomenc.c
>> > @@ -504,7 +504,8 @@ static av_cold int aom_init(AVCodecContext *avctx,
>> >      enccfg.g_h            = avctx->height;
>> >      enccfg.g_timebase.num = avctx->time_base.num;
>> >      enccfg.g_timebase.den = avctx->time_base.den;
>> > -    enccfg.g_threads      = avctx->thread_count ? avctx->thread_count :
>> > av_cpu_count();
>> > +    enccfg.g_threads      =
>> > +     FFMIN(avctx->thread_count ? avctx->thread_count : av_cpu_count(),
>> 64);
>>
>> Is there a limitation in libaom's api that requires users to never set
>> g_threads >64 that we missed so far?
>> Or is there a bug in libaom that you would like to fix in FFmpeg?
>>
>> Please explain, Carl Eugen
>>
>> I  belive this is a libaom's limitation

Then why isn't the limitation fixed in libaom?
Or is it documented?

> and we missed the check in FFmpeg part,

In which way did we "miss" it, where is it defined that it is invalid
to pass large positive numbers to this option?

> libvpx have the same issue and the commit d6b1248fc
> try to fix the same issue.

Two wrong commits don't make a correct one;-)

Carl Eugen
mypopy@gmail.com Nov. 28, 2018, 2:58 a.m.
On Wed, Nov 28, 2018 at 9:41 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-11-28 1:54 GMT+01:00, mypopy@gmail.com <mypopy@gmail.com>:
> > On Wed, Nov 28, 2018 at 4:47 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
> >
> >> 2018-11-27 10:18 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> >> > fixed the error in Intel(R) Xeon(R) Gold 6152 CPU like:
> >> > [libaom-av1 @ 0x469f340] Failed to initialize encoder: Invalid
> parameter
> >> > [libaom-av1 @ 0x469f340]   Additional information: g_threads out of
> >> > range
> >> > [..MAX_NUM_THREADS]
> >> >
> >> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> >> > ---
> >> >  libavcodec/libaomenc.c |    3 ++-
> >> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> >> > index cb31c55..ccb0cf9 100644
> >> > --- a/libavcodec/libaomenc.c
> >> > +++ b/libavcodec/libaomenc.c
> >> > @@ -504,7 +504,8 @@ static av_cold int aom_init(AVCodecContext
> *avctx,
> >> >      enccfg.g_h            = avctx->height;
> >> >      enccfg.g_timebase.num = avctx->time_base.num;
> >> >      enccfg.g_timebase.den = avctx->time_base.den;
> >> > -    enccfg.g_threads      = avctx->thread_count ? avctx
> ->thread_count :
> >> > av_cpu_count();
> >> > +    enccfg.g_threads      =
> >> > +     FFMIN(avctx->thread_count ? avctx->thread_count : a
> v_cpu_count(),
> >> 64);
> >>
> >> Is there a limitation in libaom's api that requires users to never set
> >> g_threads >64 that we missed so far?
> >> Or is there a bug in libaom that you would like to fix in FFmpeg?
> >>
> >> Please explain, Carl Eugen
> >>
> >> I  belive this is a libaom's limitation
>
> Then why isn't the limitation fixed in libaom?
> Or is it documented?
>
> This is the check in libaom:
https://aomedia.googlesource.com/aom/+/master/av1/av1_cx_iface.c#254

or you can grep "MAX_NUM_THREADS" in libaom :)



> > and we missed the check in FFmpeg part,
>
> In which way did we "miss" it, where is it defined that it is invalid
> to pass large positive numbers to this option?
>
I think we usually don't  setting the avctx->thread_count and av_cpu_count()
< 64 in most of the CPU, but in our test environment (Intel(R) Xeon(R) Gold
6152) av_cpu_count == 88, this is the reason we didn't find this issue
before this case.

>
> > libvpx have the same issue and the commit d6b1248fc
> > try to fix the same issue.
>
> Two wrong commits don't make a correct one;-)
>
> Carl Eugen
>
James Almer Nov. 30, 2018, 12:38 a.m.
On 11/27/2018 6:18 AM, Jun Zhao wrote:
> fixed the error in Intel(R) Xeon(R) Gold 6152 CPU like:
> [libaom-av1 @ 0x469f340] Failed to initialize encoder: Invalid parameter
> [libaom-av1 @ 0x469f340]   Additional information: g_threads out of range [..MAX_NUM_THREADS]
> 
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  libavcodec/libaomenc.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index cb31c55..ccb0cf9 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -504,7 +504,8 @@ static av_cold int aom_init(AVCodecContext *avctx,
>      enccfg.g_h            = avctx->height;
>      enccfg.g_timebase.num = avctx->time_base.num;
>      enccfg.g_timebase.den = avctx->time_base.den;
> -    enccfg.g_threads      = avctx->thread_count ? avctx->thread_count : av_cpu_count();
> +    enccfg.g_threads      =
> +	FFMIN(avctx->thread_count ? avctx->thread_count : av_cpu_count(), 64);
>  
>      if (ctx->lag_in_frames >= 0)
>          enccfg.g_lag_in_frames = ctx->lag_in_frames;
> 

Applied. Better to clip the value than to let initialization using
default values fail in the user's face because they have a computer with
a lot logical cores.

libaom should nonetheless either do this clipping internally, or make
this limit clear in the API and/or the documentation.

Patch hide | download patch | download mbox

diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index cb31c55..ccb0cf9 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -504,7 +504,8 @@  static av_cold int aom_init(AVCodecContext *avctx,
     enccfg.g_h            = avctx->height;
     enccfg.g_timebase.num = avctx->time_base.num;
     enccfg.g_timebase.den = avctx->time_base.den;
-    enccfg.g_threads      = avctx->thread_count ? avctx->thread_count : av_cpu_count();
+    enccfg.g_threads      =
+	FFMIN(avctx->thread_count ? avctx->thread_count : av_cpu_count(), 64);
 
     if (ctx->lag_in_frames >= 0)
         enccfg.g_lag_in_frames = ctx->lag_in_frames;