diff mbox series

[FFmpeg-devel,05/10] lavc/libopenh264enc: prompt slice number changing according to cpus

Message ID 1586171693-7836-6-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series Patch set for the enhancement of libopenh264 encoder
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Fu, Linjie April 6, 2020, 11:14 a.m. UTC
Libopenh264enc would set the slice according to the number of cpu cores
if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.

Prompt a warning for user to catch this.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/libopenh264enc.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Anton Khirnov April 10, 2020, 10:23 a.m. UTC | #1
Quoting Linjie Fu (2020-04-06 13:14:48)
> Libopenh264enc would set the slice according to the number of cpu cores
> if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.
> 
> Prompt a warning for user to catch this.
> 
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/libopenh264enc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index dab8244..01a85fb 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -237,6 +237,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      param.sSpatialLayers[0].sSliceCfg.uiSliceMode               = s->slice_mode;
>      param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum = avctx->slices;
>  #endif
> +    if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
> +        av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
> +               "default to use the number of CPU cores: %d\n", av_cpu_count());

Generally makes sense, but I'd avoid the call to av_cpu_count() since we
don't know what method precisely will libopenh264 use to set the slice
count. So IMO just say something like "slice count will be set
automatically".
Fu, Linjie April 10, 2020, 1:49 p.m. UTC | #2
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:23
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> slice number changing according to cpus
> 
> Quoting Linjie Fu (2020-04-06 13:14:48)
> > Libopenh264enc would set the slice according to the number of cpu cores
> > if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.
> >
> > Prompt a warning for user to catch this.
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> >  libavcodec/libopenh264enc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index dab8244..01a85fb 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -237,6 +237,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >      param.sSpatialLayers[0].sSliceCfg.uiSliceMode               = s->slice_mode;
> >      param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum = avctx-
> >slices;
> >  #endif
> > +    if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
> > +        av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
> > +               "default to use the number of CPU cores: %d\n", av_cpu_count());
> 
> Generally makes sense, but I'd avoid the call to av_cpu_count() since we
> don't know what method precisely will libopenh264 use to set the slice
> count. So IMO just say something like "slice count will be set
> automatically".

There is a statement in the API definition[1] for uiSliceNum which says:
 	when uiSliceNum=0 means auto design it with cpu core number.
So IMHO it seems clear enough if I got it correctly.

I'm good with this suggestion and will update, since the detailed slice number
would also be dumped in debug level as well (however a little bit mixed up with
all the debug info), thx.

- Linjie

[1] https://github.com/cisco/openh264/blob/master/codec/api/svc/codec_app_def.h#L351
Anton Khirnov April 11, 2020, 8:38 a.m. UTC | #3
Quoting Fu, Linjie (2020-04-10 15:49:30)
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Anton Khirnov
> > Sent: Friday, April 10, 2020 18:23
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> > slice number changing according to cpus
> > 
> > Quoting Linjie Fu (2020-04-06 13:14:48)
> > > Libopenh264enc would set the slice according to the number of cpu cores
> > > if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.
> > >
> > > Prompt a warning for user to catch this.
> > >
> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > ---
> > >  libavcodec/libopenh264enc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > index dab8244..01a85fb 100644
> > > --- a/libavcodec/libopenh264enc.c
> > > +++ b/libavcodec/libopenh264enc.c
> > > @@ -237,6 +237,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > >      param.sSpatialLayers[0].sSliceCfg.uiSliceMode               = s->slice_mode;
> > >      param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum = avctx-
> > >slices;
> > >  #endif
> > > +    if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
> > > +        av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
> > > +               "default to use the number of CPU cores: %d\n", av_cpu_count());
> > 
> > Generally makes sense, but I'd avoid the call to av_cpu_count() since we
> > don't know what method precisely will libopenh264 use to set the slice
> > count. So IMO just say something like "slice count will be set
> > automatically".
> 
> There is a statement in the API definition[1] for uiSliceNum which says:
>         when uiSliceNum=0 means auto design it with cpu core number.
> So IMHO it seems clear enough if I got it correctly.

My concern is that different methods of computing the core count might
return different results in certain cases. E.g. if the code is running
under some resource management system that restricts it to a subset of
total cores on the system. So we should not assume that av_cpu_count()
will always necessarily return the same number that libopenh264 uses.
Fu, Linjie April 11, 2020, 9:38 a.m. UTC | #4
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton Khirnov
> Sent: Saturday, April 11, 2020 16:38
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> slice number changing according to cpus
> 
> Quoting Fu, Linjie (2020-04-10 15:49:30)
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Anton Khirnov
> > > Sent: Friday, April 10, 2020 18:23
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> > > slice number changing according to cpus
> > >
> > > Quoting Linjie Fu (2020-04-06 13:14:48)
> > > > Libopenh264enc would set the slice according to the number of cpu
> cores
> > > > if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.
> > > >
> > > > Prompt a warning for user to catch this.
> > > >
> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > > ---
> > > >  libavcodec/libopenh264enc.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > > index dab8244..01a85fb 100644
> > > > --- a/libavcodec/libopenh264enc.c
> > > > +++ b/libavcodec/libopenh264enc.c
> > > > @@ -237,6 +237,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > >      param.sSpatialLayers[0].sSliceCfg.uiSliceMode               = s-
> >slice_mode;
> > > >      param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum =
> avctx-
> > > >slices;
> > > >  #endif
> > > > +    if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
> > > > +        av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
> > > > +               "default to use the number of CPU cores: %d\n",
> av_cpu_count());
> > >
> > > Generally makes sense, but I'd avoid the call to av_cpu_count() since we
> > > don't know what method precisely will libopenh264 use to set the slice
> > > count. So IMO just say something like "slice count will be set
> > > automatically".
> >
> > There is a statement in the API definition[1] for uiSliceNum which says:
> >         when uiSliceNum=0 means auto design it with cpu core number.
> > So IMHO it seems clear enough if I got it correctly.
> 
> My concern is that different methods of computing the core count might
> return different results in certain cases. E.g. if the code is running
> under some resource management system that restricts it to a subset of
> total cores on the system. So we should not assume that av_cpu_count()
> will always necessarily return the same number that libopenh264 uses.

Indeed, thanks for the elaborations.

- Linjie
diff mbox series

Patch

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index dab8244..01a85fb 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -237,6 +237,9 @@  FF_ENABLE_DEPRECATION_WARNINGS
     param.sSpatialLayers[0].sSliceCfg.uiSliceMode               = s->slice_mode;
     param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum = avctx->slices;
 #endif
+    if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
+        av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
+               "default to use the number of CPU cores: %d\n", av_cpu_count());
 
     if (s->slice_mode == SM_SIZELIMITED_SLICE) {
         if (s->max_nal_size) {