Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate
> From: Martin Storsjö
> Sent: Tuesday, April 28, 2020 16:08
> To: Fu, Linjie
> Cc: FFmpeg development discussions and patches de...@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ö
> >> Sent: Tuesday, April 28, 2020 14:28
> >> To: Fu, Linjie
> >> Cc: FFmpeg development discussions and patches >> de...@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ö
> >>>> 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 " 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
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ö
Sent: Tuesday, April 28, 2020 14:28
To: Fu, Linjie
Cc: FFmpeg development discussions and patches
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ö
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 " 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate
> From: Martin Storsjö
> Sent: Tuesday, April 28, 2020 14:28
> To: Fu, Linjie
> Cc: FFmpeg development discussions and patches de...@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ö
> >> 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 " 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
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ö
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 " 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate
> From: Martin Storsjö
> 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 " 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate
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
---
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
