diff mbox series

[FFmpeg-devel,v4,2/9] lavc/libopenh264enc: add default gop size and bit rate

Message ID 1586926427-20170-3-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series Enhancement for libopenh264 encoder
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Fu, Linjie April 15, 2020, 4:53 a.m. UTC
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(-)

Comments

Martin Storsjö April 27, 2020, 7:28 p.m. UTC | #1
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
Fu, Linjie April 28, 2020, 6:21 a.m. UTC | #2
> 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
Martin Storsjö April 28, 2020, 6:28 a.m. UTC | #3
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
Fu, Linjie April 28, 2020, 7:36 a.m. UTC | #4
> 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
Martin Storsjö April 28, 2020, 8:08 a.m. UTC | #5
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
Fu, Linjie April 28, 2020, 9:19 a.m. UTC | #6
> 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 mbox series

Patch

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 },