| Message ID | 1586926427-20170-3-git-send-email-linjie.fu@intel.com |
|---|---|
| State | Superseded |
| 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: > It would be 200kbps bitrate with gop size = 12 by default > which generated too many IDR frames in rather low bit rate. > The quality would be poor. > > Set these default values according to vaapi encoder, and > use 2Mbps bitrate if user doesn't set it explicitly as > nvenc sugguested. > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > libavcodec/libopenh264enc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c > index c7ae5b1..57313b1 100644 > --- a/libavcodec/libopenh264enc.c > +++ b/libavcodec/libopenh264enc.c > @@ -132,7 +132,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > param.fMaxFrameRate = 1/av_q2d(avctx->time_base); > param.iPicWidth = avctx->width; > param.iPicHeight = avctx->height; > - param.iTargetBitrate = avctx->bit_rate; > + param.iTargetBitrate = avctx->bit_rate > 0 ? avctx->bit_rate : 2*1000*1000; > 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) > @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, > } > > static const AVCodecDefault svc_enc_defaults[] = { > + { "b", "0" }, > + { "g", "120" }, > { "qmin", "-1" }, Why do you hardcode a value for g here, but put the default bitrate value in the code above? Wouldn't it be clearer to have both defaults here at the same place? // Martin
> From: Martin Storsjö <martin@martin.st> > Sent: Tuesday, April 28, 2020 03:28 > > static const AVCodecDefault svc_enc_defaults[] = { > > + { "b", "0" }, > > + { "g", "120" }, > > { "qmin", "-1" }, > > Why do you hardcode a value for g here, but put the default bitrate value > in the code above? Wouldn't it be clearer to have both defaults here at > the same place? > A default value in svc_enc_defaults[] would help to distinguish between "the user specified the bitrate to be <x>" vs. "the user did not specify anything about the target bitrate", as Anton has suggested in [1]. Considering about your suggestions in patch 1/9, IMO it seems to be more reasonable to keep the uiIntraPeriod untouched. The libopenh264 library would fill the default value of uiIntraPeriod to 0, and as a consequence the gop size would be rather large. Updated the default "g" to "-1", same as libx264 did. (Note that it's not acceptable for bitrate, since bitrate = 0 as default in library is not valid) - Linjie [1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260320.html
On Tue, 28 Apr 2020, Fu, Linjie wrote: >> From: Martin Storsjö <martin@martin.st> >> Sent: Tuesday, April 28, 2020 03:28 >>> static const AVCodecDefault svc_enc_defaults[] = { >>> + { "b", "0" }, >>> + { "g", "120" }, >>> { "qmin", "-1" }, >> >> Why do you hardcode a value for g here, but put the default bitrate value >> in the code above? Wouldn't it be clearer to have both defaults here at >> the same place? >> > A default value in svc_enc_defaults[] would help to distinguish between > "the user specified the bitrate to be <x>" vs. "the user did not specify anything > about the target bitrate", as Anton has suggested in [1]. > > Considering about your suggestions in patch 1/9, IMO it seems to be more reasonable > to keep the uiIntraPeriod untouched. The libopenh264 library would fill the default > value of uiIntraPeriod to 0, and as a consequence the gop size would be rather large. > > Updated the default "g" to "-1", same as libx264 did. > (Note that it's not acceptable for bitrate, since bitrate = 0 as default in library is not > valid) If we have the option of not setting the bitrate on the encoder, then yes, it's better to avoid setting one in svc_enc_defaults. But in this case, if no bitrate was chosen by the user, we end up enforcing a default value anyway - just that the default is not written in the global defaults table (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in code. If we actually need to set a bitrate, it's IMO better to put this default in the table, especially if we have other defaults like gop size there. // Martin
> From: Martin Storsjö <martin@martin.st> > Sent: Tuesday, April 28, 2020 14:28 > To: Fu, Linjie <linjie.fu@intel.com> > Cc: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add > default gop size and bit rate > > On Tue, 28 Apr 2020, Fu, Linjie wrote: > > >> From: Martin Storsjö <martin@martin.st> > >> Sent: Tuesday, April 28, 2020 03:28 > >>> static const AVCodecDefault svc_enc_defaults[] = { > >>> + { "b", "0" }, > >>> + { "g", "120" }, > >>> { "qmin", "-1" }, > >> > >> Why do you hardcode a value for g here, but put the default bitrate value > >> in the code above? Wouldn't it be clearer to have both defaults here at > >> the same place? > >> > > A default value in svc_enc_defaults[] would help to distinguish between > > "the user specified the bitrate to be <x>" vs. "the user did not specify > anything > > about the target bitrate", as Anton has suggested in [1]. > > > > Considering about your suggestions in patch 1/9, IMO it seems to be more > reasonable > > to keep the uiIntraPeriod untouched. The libopenh264 library would fill the > default > > value of uiIntraPeriod to 0, and as a consequence the gop size would be > rather large. > > > > Updated the default "g" to "-1", same as libx264 did. > > (Note that it's not acceptable for bitrate, since bitrate = 0 as default in > library is not > > valid) > > If we have the option of not setting the bitrate on the encoder, then yes, > it's better to avoid setting one in svc_enc_defaults. But in this case, if > no bitrate was chosen by the user, we end up enforcing a default value > anyway - just that the default is not written in the global defaults table > (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in > code. > > If we actually need to set a bitrate, it's IMO better to put this default Indeed we have to set the bitrate. > in the table, especially if we have other defaults like gop size there. > In previous discussion in [1], we seems to come to an alignment that this would help to determine whether it's specified by user, and hence allow libopenh264enc to select the RC_MODE through settings like avctx->bit_rate, max/min/avg bitrates if rc_mode is not set explicitly. VAAPI encoder has the similar logic in [2]. I'm okay with either solution, if above logic is not going to be implemented. [1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260215.html [2] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/vaapi_encode.c#L1542 - Linjie
On Tue, 28 Apr 2020, Fu, Linjie wrote: >> From: Martin Storsjö <martin@martin.st> >> Sent: Tuesday, April 28, 2020 14:28 >> To: Fu, Linjie <linjie.fu@intel.com> >> Cc: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add >> default gop size and bit rate >> >> On Tue, 28 Apr 2020, Fu, Linjie wrote: >> >>>> From: Martin Storsjö <martin@martin.st> >>>> Sent: Tuesday, April 28, 2020 03:28 >>>>> static const AVCodecDefault svc_enc_defaults[] = { >>>>> + { "b", "0" }, >>>>> + { "g", "120" }, >>>>> { "qmin", "-1" }, >>>> >>>> Why do you hardcode a value for g here, but put the default bitrate value >>>> in the code above? Wouldn't it be clearer to have both defaults here at >>>> the same place? >>>> >>> A default value in svc_enc_defaults[] would help to distinguish between >>> "the user specified the bitrate to be <x>" vs. "the user did not specify >> anything >>> about the target bitrate", as Anton has suggested in [1]. >>> >>> Considering about your suggestions in patch 1/9, IMO it seems to be more >> reasonable >>> to keep the uiIntraPeriod untouched. The libopenh264 library would fill the >> default >>> value of uiIntraPeriod to 0, and as a consequence the gop size would be >> rather large. >>> >>> Updated the default "g" to "-1", same as libx264 did. >>> (Note that it's not acceptable for bitrate, since bitrate = 0 as default in >> library is not >>> valid) >> >> If we have the option of not setting the bitrate on the encoder, then yes, >> it's better to avoid setting one in svc_enc_defaults. But in this case, if >> no bitrate was chosen by the user, we end up enforcing a default value >> anyway - just that the default is not written in the global defaults table >> (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in >> code. >> >> If we actually need to set a bitrate, it's IMO better to put this default > > Indeed we have to set the bitrate. > >> in the table, especially if we have other defaults like gop size there. >> > > In previous discussion in [1], we seems to come to an alignment that this would > help to determine whether it's specified by user, and hence allow libopenh264enc > to select the RC_MODE through settings like avctx->bit_rate, max/min/avg bitrates > if rc_mode is not set explicitly. > > VAAPI encoder has the similar logic in [2]. > > I'm okay with either solution, if above logic is not going to be implemented. Right, so if logic that selects rc mode based on those settings will be implemented, then yes it makes sense to keep the default at -1 and fall back to a default bitrate within code. In that case I'd kind of prefer to have the default bitrate specified in a define high up in the file, instead of buried in init logic though. // Martin
> From: Martin Storsjö <martin@martin.st> > Sent: Tuesday, April 28, 2020 16:08 > To: Fu, Linjie <linjie.fu@intel.com> > Cc: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add > default gop size and bit rate > > On Tue, 28 Apr 2020, Fu, Linjie wrote: > > >> From: Martin Storsjö <martin@martin.st> > >> Sent: Tuesday, April 28, 2020 14:28 > >> To: Fu, Linjie <linjie.fu@intel.com> > >> Cc: FFmpeg development discussions and patches <ffmpeg- > >> devel@ffmpeg.org> > >> Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add > >> default gop size and bit rate > >> > >> On Tue, 28 Apr 2020, Fu, Linjie wrote: > >> > >>>> From: Martin Storsjö <martin@martin.st> > >>>> Sent: Tuesday, April 28, 2020 03:28 > >>>>> static const AVCodecDefault svc_enc_defaults[] = { > >>>>> + { "b", "0" }, > >>>>> + { "g", "120" }, > >>>>> { "qmin", "-1" }, > >>>> > >>>> Why do you hardcode a value for g here, but put the default bitrate > value > >>>> in the code above? Wouldn't it be clearer to have both defaults here at > >>>> the same place? > >>>> > >>> A default value in svc_enc_defaults[] would help to distinguish between > >>> "the user specified the bitrate to be <x>" vs. "the user did not specify > >> anything > >>> about the target bitrate", as Anton has suggested in [1]. > >>> > >>> Considering about your suggestions in patch 1/9, IMO it seems to be > more > >> reasonable > >>> to keep the uiIntraPeriod untouched. The libopenh264 library would fill > the > >> default > >>> value of uiIntraPeriod to 0, and as a consequence the gop size would be > >> rather large. > >>> > >>> Updated the default "g" to "-1", same as libx264 did. > >>> (Note that it's not acceptable for bitrate, since bitrate = 0 as default in > >> library is not > >>> valid) > >> > >> If we have the option of not setting the bitrate on the encoder, then yes, > >> it's better to avoid setting one in svc_enc_defaults. But in this case, if > >> no bitrate was chosen by the user, we end up enforcing a default value > >> anyway - just that the default is not written in the global defaults table > >> (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in > >> code. > >> > >> If we actually need to set a bitrate, it's IMO better to put this default > > > > Indeed we have to set the bitrate. > > > >> in the table, especially if we have other defaults like gop size there. > >> > > > > In previous discussion in [1], we seems to come to an alignment that this > would > > help to determine whether it's specified by user, and hence allow > libopenh264enc > > to select the RC_MODE through settings like avctx->bit_rate, max/min/avg > bitrates > > if rc_mode is not set explicitly. > > > > VAAPI encoder has the similar logic in [2]. > > > > I'm okay with either solution, if above logic is not going to be implemented. > > Right, so if logic that selects rc mode based on those settings will be > implemented, then yes it makes sense to keep the default at -1 and fall > back to a default bitrate within code. In that case I'd kind of prefer to > have the default bitrate specified in a define high up in the file, > instead of buried in init logic though. > > // Martin Updated. Separated the patch set and resend, thanks for the suggestions. - Linjie
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c index c7ae5b1..57313b1 100644 --- a/libavcodec/libopenh264enc.c +++ b/libavcodec/libopenh264enc.c @@ -132,7 +132,7 @@ FF_ENABLE_DEPRECATION_WARNINGS param.fMaxFrameRate = 1/av_q2d(avctx->time_base); param.iPicWidth = avctx->width; param.iPicHeight = avctx->height; - param.iTargetBitrate = avctx->bit_rate; + param.iTargetBitrate = avctx->bit_rate > 0 ? avctx->bit_rate : 2*1000*1000; 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) @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, } static const AVCodecDefault svc_enc_defaults[] = { + { "b", "0" }, + { "g", "120" }, { "qmin", "-1" }, { "qmax", "-1" }, { NULL },
It would be 200kbps bitrate with gop size = 12 by default which generated too many IDR frames in rather low bit rate. The quality would be poor. Set these default values according to vaapi encoder, and use 2Mbps bitrate if user doesn't set it explicitly as nvenc sugguested. Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- libavcodec/libopenh264enc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)