[FFmpeg-devel,v2] libsvtav1: Add logical_processors option

From: Christopher Degawa <christopher.degawa@intel.com>

Used for limiting the size of memory buffers and threads for a target
logical processor count, but does not set thread affinity or limit the
amount of threads used, although thread affinities can be controlled
with an additional parameters, it is prefered to add them until
a svtav1-params option or similar is added

Signed-off-by: Christopher Degawa <christopher.degawa@intel.com>
---
 doc/encoders.texi      | 5 +++++
 libavcodec/libsvtav1.c | 7 +++++++
 2 files changed, 12 insertions(+)

On Sat, Feb 20, 2021 at 3:20 AM Christopher Degawa <ccom@randomderp.com> wrote:
>
> From: Christopher Degawa <christopher.degawa@intel.com>
>
> Used for limiting the size of memory buffers and threads for a target
> logical processor count, but does not set thread affinity or limit the
> amount of threads used, although thread affinities can be controlled
> with an additional parameters, it is prefered to add them until
> a svtav1-params option or similar is added
>
> Signed-off-by: Christopher Degawa <christopher.degawa@intel.com>
> ---
>  doc/encoders.texi      | 5 +++++
>  libavcodec/libsvtav1.c | 7 +++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/doc/encoders.texi b/doc/encoders.texi
> index 8fb573c416..28c1a11a6c 100644
> --- a/doc/encoders.texi
> +++ b/doc/encoders.texi
> @@ -1757,6 +1757,11 @@ Set log2 of the number of rows of tiles to use (0-6).
>  @item tile_columns
>  Set log2 of the number of columns of tiles to use (0-4).
>
> +@item logical_processors
> +Number of logical processors to run the encoder on, threads are managed by the OS scheduler.
> +Used for limiting the size of memory buffers and threads for a target logical processor count.
> +Does not set thread affinity or limit threads.
> +

Hi, and sorry for getting to this patch so slowly. Thank you for your
discussion on IRC, I think that cleared up what this option does a
bit.

I think it might be more clear to call this just a "thread count
multiplier override to limit the amount of threads utilized" in the
docs and the help string, since that's what it essentially seems to
boil to. According to what I grasped from your explanation, the
encoder spawns XYZ threads per logical processor (core) by default,
and this is a way to limit that calculation to a specific number. As
SVT-AV1 does not have a separate switch to just plain set the amount
of threads in general, it is also the only way right now to limit the
thread count at the moment.

ref:
https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/05ed7cb78620c2ebbc948b20f26dd9a9c1756fa5/Source/Lib/Encoder/Globals/EbEncHandle.c#L254-257

>  @end table
>
>  @section libkvazaar
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index eb6043bcac..2296735f25 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -71,6 +71,8 @@ typedef struct SvtContext {
>
>      int tile_columns;
>      int tile_rows;
> +
> +    unsigned logical_processors;
>  } SvtContext;
>
>  static const struct {
> @@ -218,6 +220,8 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>      param->tile_columns = svt_enc->tile_columns;
>      param->tile_rows    = svt_enc->tile_rows;
>
> +    param->logical_processors = svt_enc->logical_processors;
> +

Do we already require a new enough SVT-AV1 to always have this option?
If yes, great. If not, it might be OK to just bump the requirement
then (to keep unnecessary ifdefs at bay for a relatively new and
actively developed library)?

>      return 0;
>  }
>
> @@ -533,6 +537,9 @@ static const AVOption options[] = {
>      { "tile_columns", "Log2 of number of tile columns to use", OFFSET(tile_columns), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 4, VE},
>      { "tile_rows", "Log2 of number of tile rows to use", OFFSET(tile_rows), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 6, VE},
>
> +    { "logical_processors", "Number of logical processors to run the encoder on, threads are managed by the OS scheduler", OFFSET(logical_processors),
> +      AV_OPT_TYPE_INT, { .i64 = 0 }, 0,  INT_MAX, VE },
> +

I think this could just be made to mention that it's a thread count
multiplier override to limit the amount of threads utilized.

There is an int/unsigned mismatch, but I think that should be OK since
you limit the value to 0-INT_MAX in the AVOption itself?

Otherwise LGTM, and once again sorry for taking the time to get to this.

Jan

On Tue, Mar 30, 2021 at 4:53 PM Jan Ekström <jeebjp@gmail.com> wrote:

> > @@ -218,6 +220,8 @@ static int
> config_enc_params(EbSvtAv1EncConfiguration *param,
> >      param->tile_columns = svt_enc->tile_columns;
> >      param->tile_rows    = svt_enc->tile_rows;
> >
> > +    param->logical_processors = svt_enc->logical_processors;
> > +
>
> Do we already require a new enough SVT-AV1 to always have this option?
> If yes, great. If not, it might be OK to just bump the requirement
> then (to keep unnecessary ifdefs at bay for a relatively new and
> actively developed library)?
>

afaik, this option has existed before the ffmpeg wrapper was upstreamed, so
I do not think any ifdefs are needed

ref:
https://gitlab.com/AOMediaCodec/SVT-AV1/-/commit/a6c1f81989c6cab0f477adfa867d5ff3dad2725c


> > +    { "logical_processors", "Number of logical processors to run the
> encoder on, threads are managed by the OS scheduler",
> OFFSET(logical_processors),
> > +      AV_OPT_TYPE_INT, { .i64 = 0 }, 0,  INT_MAX, VE },
> > +
>
> I think this could just be made to mention that it's a thread count
> multiplier override to limit the amount of threads utilized.
>

after discussing this option on IRC, I think the wording of thread count
multiplier would probably fit better, something along the lines of

> { "logical_processors", "Thread count multiplier with a max of the number
of logical cores available",

I will resubmit an updated patch soon


> There is an int/unsigned mismatch, but I think that should be OK since
> you limit the value to 0-INT_MAX in the AVOption itself?
>

changed to

> AV_OPT_TYPE_INT, { .i64 = 0 }, 0,  UINT_MAX, VE },


>
> Otherwise LGTM, and once again sorry for taking the time to get to this.
>
Thank you