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

[Jan Ekström]

On Sat, Feb 20, 2021 at 3:20 AM Christopher Degawa <ccom at randomderp.com> wrote:
>
> From: Christopher Degawa <christopher.degawa at 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 at 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).
>
> + at 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