| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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".
> 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
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.
> 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 --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) {
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(+)