| Message ID | 1586926427-20170-2-git-send-email-linjie.fu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enhancement for libopenh264 encoder | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Wed, 15 Apr 2020, Linjie Fu wrote: > Set default QP range to (1, 51) instead of (2, 32). > > QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp > equals 0, the QP range would be changed unexpectedly inside libopenh264 > with a warning: > > Warning:Change QP Range from(0,51) to (12,42) > > [1] <https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375> > [2] <https://github.com/cisco/openh264/issues/3259> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > libavcodec/libopenh264enc.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c > index dd5d4ee..c7ae5b1 100644 > --- a/libavcodec/libopenh264enc.c > +++ b/libavcodec/libopenh264enc.c > @@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > param.iTargetBitrate = avctx->bit_rate; > param.iMaxBitrate = FFMAX(avctx->rc_max_rate, avctx->bit_rate); > param.iRCMode = RC_QUALITY_MODE; > + // QP = 0 is not well supported, so default to (1, 51) > + param.iMaxQp = avctx->qmax >= 0 ? av_clip(avctx->qmax, 1, 51) : 51; > + param.iMinQp = avctx->qmin >= 0 ? av_clip(avctx->qmin, 1, param.iMaxQp) : 1; If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better to not touch param.iMax/MinQp at all (and use the default value of the library, which may change between versions), instead of overriding it with a value hardcoded here? // Martin
> From: Martin Storsjö <martin@martin.st> > Sent: Tuesday, April 28, 2020 03:27 > > If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better > to not touch param.iMax/MinQp at all (and use the default value of the > library, which may change between versions), instead of overriding it with > a value hardcoded here? > Okay, this seems more natural if the recommended QP range varies between versions, though one of my original purposes is to avoid the warning in default situation for changing the QP inside libopenh264 library. Updated locally as suggested, and thanks for the review for the whole set. - Linjie
On Tue, 28 Apr 2020, Fu, Linjie wrote: >> From: Martin Storsjö <martin@martin.st> >> Sent: Tuesday, April 28, 2020 03:27 >> >> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better >> to not touch param.iMax/MinQp at all (and use the default value of the >> library, which may change between versions), instead of overriding it with >> a value hardcoded here? >> > Okay, this seems more natural if the recommended QP range varies between > versions, though one of my original purposes is to avoid the warning in default > situation for changing the QP inside libopenh264 library. Well in general I'd want to avoid hardcoding opinionated defaults within our own wrapper - I'd like it to behave as close to what upstream intended, so that whatever issues we see with defaults, are the same issues that everyone else sees as well, so any fixes to those defaults upstream also end up for us - so we don't get stuck on whatever we thought was a good default at some point. What warnings about changing QP are you referring to? // Martin
> From: Martin Storsjö <martin@martin.st> > Sent: Tuesday, April 28, 2020 14:19 > To: Fu, Linjie <linjie.fu@intel.com> > Cc: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add > default qmin/qmax support > > On Tue, 28 Apr 2020, Fu, Linjie wrote: > > >> From: Martin Storsjö <martin@martin.st> > >> Sent: Tuesday, April 28, 2020 03:27 > >> > >> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better > >> to not touch param.iMax/MinQp at all (and use the default value of the > >> library, which may change between versions), instead of overriding it with > >> a value hardcoded here? > >> > > Okay, this seems more natural if the recommended QP range varies > between > > versions, though one of my original purposes is to avoid the warning in > default > > situation for changing the QP inside libopenh264 library. > > Well in general I'd want to avoid hardcoding opinionated defaults within > our own wrapper - I'd like it to behave as close to what upstream > intended, so that whatever issues we see with defaults, are the same > issues that everyone else sees as well, so any fixes to those defaults > upstream also end up for us - so we don't get stuck on whatever we thought > was a good default at some point. Agree, this makes more sense. > What warnings about changing QP are you referring to? Warning:Change QP Range from(0,51) to (12,42) https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375 The main reason is that "0" is not supported well, which is the default value of iMinQp. - Linjie
On Tue, 28 Apr 2020, Fu, Linjie wrote: >> From: Martin Storsjö <martin@martin.st> >> Sent: Tuesday, April 28, 2020 14:19 >> To: Fu, Linjie <linjie.fu@intel.com> >> Cc: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add >> default qmin/qmax support >> >> On Tue, 28 Apr 2020, Fu, Linjie wrote: >> >>>> From: Martin Storsjö <martin@martin.st> >>>> Sent: Tuesday, April 28, 2020 03:27 >>>> >>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better >>>> to not touch param.iMax/MinQp at all (and use the default value of the >>>> library, which may change between versions), instead of overriding it with >>>> a value hardcoded here? >>>> >>> Okay, this seems more natural if the recommended QP range varies >> between >>> versions, though one of my original purposes is to avoid the warning in >> default >>> situation for changing the QP inside libopenh264 library. >> >> Well in general I'd want to avoid hardcoding opinionated defaults within >> our own wrapper - I'd like it to behave as close to what upstream >> intended, so that whatever issues we see with defaults, are the same >> issues that everyone else sees as well, so any fixes to those defaults >> upstream also end up for us - so we don't get stuck on whatever we thought >> was a good default at some point. > > Agree, this makes more sense. > >> What warnings about changing QP are you referring to? > Warning:Change QP Range from(0,51) to (12,42) > https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375 > > The main reason is that "0" is not supported well, which is the default > value of iMinQp. Ah, right. In that case, setting some other default might make sense indeed. How/where does OpenH264 set suitable values for this, in order not to get the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere in every caller? // Martin
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Martin Storsjö > Sent: Tuesday, April 28, 2020 14:31 > To: Fu, Linjie <linjie.fu@intel.com> > Cc: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add > default qmin/qmax support > > On Tue, 28 Apr 2020, Fu, Linjie wrote: > > >> From: Martin Storsjö <martin@martin.st> > >> Sent: Tuesday, April 28, 2020 14:19 > >> To: Fu, Linjie <linjie.fu@intel.com> > >> Cc: FFmpeg development discussions and patches <ffmpeg- > >> devel@ffmpeg.org> > >> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add > >> default qmin/qmax support > >> > >> On Tue, 28 Apr 2020, Fu, Linjie wrote: > >> > >>>> From: Martin Storsjö <martin@martin.st> > >>>> Sent: Tuesday, April 28, 2020 03:27 > >>>> > >>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be > better > >>>> to not touch param.iMax/MinQp at all (and use the default value of the > >>>> library, which may change between versions), instead of overriding it > with > >>>> a value hardcoded here? > >>>> > >>> Okay, this seems more natural if the recommended QP range varies > >> between > >>> versions, though one of my original purposes is to avoid the warning in > >> default > >>> situation for changing the QP inside libopenh264 library. > >> > >> Well in general I'd want to avoid hardcoding opinionated defaults within > >> our own wrapper - I'd like it to behave as close to what upstream > >> intended, so that whatever issues we see with defaults, are the same > >> issues that everyone else sees as well, so any fixes to those defaults > >> upstream also end up for us - so we don't get stuck on whatever we > thought > >> was a good default at some point. > > > > Agree, this makes more sense. > > > >> What warnings about changing QP are you referring to? > > Warning:Change QP Range from(0,51) to (12,42) > > > https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e > ncoder_ext.cpp#L375 > > > > The main reason is that "0" is not supported well, which is the default > > value of iMinQp. > > Ah, right. > > In that case, setting some other default might make sense indeed. > How/where does OpenH264 set suitable values for this, in order not to get > the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere > in > every caller? > IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library function ParamValidation() in [1]: For SCREEN_CONTENT_REAL_TIME, it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35); For other usage like CAMERA_VIDEO_REAL_TIME by default, It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42) - Linjie [1] https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379
On Tue, 28 Apr 2020, Fu, Linjie wrote: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Martin Storsjö >> Sent: Tuesday, April 28, 2020 14:31 >> To: Fu, Linjie <linjie.fu@intel.com> >> Cc: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add >> default qmin/qmax support >> >> On Tue, 28 Apr 2020, Fu, Linjie wrote: >> >>>> From: Martin Storsjö <martin@martin.st> >>>> Sent: Tuesday, April 28, 2020 14:19 >>>> To: Fu, Linjie <linjie.fu@intel.com> >>>> Cc: FFmpeg development discussions and patches <ffmpeg- >>>> devel@ffmpeg.org> >>>> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add >>>> default qmin/qmax support >>>> >>>> On Tue, 28 Apr 2020, Fu, Linjie wrote: >>>> >>>>>> From: Martin Storsjö <martin@martin.st> >>>>>> Sent: Tuesday, April 28, 2020 03:27 >>>>>> >>>>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be >> better >>>>>> to not touch param.iMax/MinQp at all (and use the default value of the >>>>>> library, which may change between versions), instead of overriding it >> with >>>>>> a value hardcoded here? >>>>>> >>>>> Okay, this seems more natural if the recommended QP range varies >>>> between >>>>> versions, though one of my original purposes is to avoid the warning in >>>> default >>>>> situation for changing the QP inside libopenh264 library. >>>> >>>> Well in general I'd want to avoid hardcoding opinionated defaults within >>>> our own wrapper - I'd like it to behave as close to what upstream >>>> intended, so that whatever issues we see with defaults, are the same >>>> issues that everyone else sees as well, so any fixes to those defaults >>>> upstream also end up for us - so we don't get stuck on whatever we >> thought >>>> was a good default at some point. >>> >>> Agree, this makes more sense. >>> >>>> What warnings about changing QP are you referring to? >>> Warning:Change QP Range from(0,51) to (12,42) >>> >> https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e >> ncoder_ext.cpp#L375 >>> >>> The main reason is that "0" is not supported well, which is the default >>> value of iMinQp. >> >> Ah, right. >> >> In that case, setting some other default might make sense indeed. >> How/where does OpenH264 set suitable values for this, in order not to get >> the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere >> in >> every caller? >> > IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library function > ParamValidation() in [1]: > For SCREEN_CONTENT_REAL_TIME, > it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35); > For other usage like CAMERA_VIDEO_REAL_TIME by default, > It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42) > > - Linjie > > [1] https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379 Right, so looking at that code, it looks like it has a good intent - if the user hasn't specified that he wants a custom setting of min/max qp, then it uses the defaults. So the problem is that this writes a warning in the log - and you want to avoid the warning. Wouldn't it just be better to change this message to WELS_LOG_INFO within openh264, to avoid it being treated as a warning - as it's not a fault, it's the intended behaviour of picking sensible defaults when the user hasn't requested anything in particular? // Martin
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c index dd5d4ee..c7ae5b1 100644 --- a/libavcodec/libopenh264enc.c +++ b/libavcodec/libopenh264enc.c @@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS param.iTargetBitrate = avctx->bit_rate; param.iMaxBitrate = FFMAX(avctx->rc_max_rate, avctx->bit_rate); param.iRCMode = RC_QUALITY_MODE; + // QP = 0 is not well supported, so default to (1, 51) + param.iMaxQp = avctx->qmax >= 0 ? av_clip(avctx->qmax, 1, 51) : 51; + param.iMinQp = avctx->qmin >= 0 ? av_clip(avctx->qmin, 1, param.iMaxQp) : 1; param.iTemporalLayerNum = 1; param.iSpatialLayerNum = 1; param.bEnableDenoise = 0; @@ -331,6 +334,12 @@ static int svc_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, return 0; } +static const AVCodecDefault svc_enc_defaults[] = { + { "qmin", "-1" }, + { "qmax", "-1" }, + { NULL }, +}; + AVCodec ff_libopenh264_encoder = { .name = "libopenh264", .long_name = NULL_IF_CONFIG_SMALL("OpenH264 H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10"), @@ -344,6 +353,7 @@ AVCodec ff_libopenh264_encoder = { .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, .pix_fmts = (const enum AVPixelFormat[]){ AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE }, + .defaults = svc_enc_defaults, .priv_class = &class, .wrapper_name = "libopenh264", };
Set default QP range to (1, 51) instead of (2, 32). QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp equals 0, the QP range would be changed unexpectedly inside libopenh264 with a warning: Warning:Change QP Range from(0,51) to (12,42) [1] <https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375> [2] <https://github.com/cisco/openh264/issues/3259> Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- libavcodec/libopenh264enc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)