diff mbox series

[FFmpeg-devel,03/10] lavc/libopenh264enc: add default gop size and bit rate

Message ID 1586171693-7836-4-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series Patch set for the enhancement of libopenh264 encoder
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Fu, Linjie April 6, 2020, 11:14 a.m. UTC
Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/libopenh264enc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Anton Khirnov April 10, 2020, 10:14 a.m. UTC | #1
Quoting Linjie Fu (2020-04-06 13:14:46)
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/libopenh264enc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index c7ae5b1..3ff5be7 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
>  }
>  
>  static const AVCodecDefault svc_enc_defaults[] = {
> +    { "b",         "2M"    },
> +    { "g",         "120"   },

Why these values specifically? What happens if we leave them unset?
Fu, Linjie April 10, 2020, 1:33 p.m. UTC | #2
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:14
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> Quoting Linjie Fu (2020-04-06 13:14:46)
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> >  libavcodec/libopenh264enc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index c7ae5b1..3ff5be7 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext
> *avctx, AVPacket *avpkt,
> >  }
> >
> >  static const AVCodecDefault svc_enc_defaults[] = {
> > +    { "b",         "2M"    },
> > +    { "g",         "120"   },
> 
> Why these values specifically? 

According to the default settings in nvenc (b = 2M) and vaapi encoder for h264 (g = 120).

>What happens if we leave them unset?

It would be 200kbps bitrate with gop size = 12.

There would be too much IDR frames with in small bitrate, which leads to
a poor quality. 

 (I should have added such statements in the commit message)

- Linjie
Anton Khirnov April 11, 2020, 8:42 a.m. UTC | #3
Quoting Fu, Linjie (2020-04-10 15:33:04)
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Anton Khirnov
> > Sent: Friday, April 10, 2020 18:14
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> > default gop size and bit rate
> > 
> > Quoting Linjie Fu (2020-04-06 13:14:46)
> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > ---
> > >  libavcodec/libopenh264enc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > index c7ae5b1..3ff5be7 100644
> > > --- a/libavcodec/libopenh264enc.c
> > > +++ b/libavcodec/libopenh264enc.c
> > > @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext
> > *avctx, AVPacket *avpkt,
> > >  }
> > >
> > >  static const AVCodecDefault svc_enc_defaults[] = {
> > > +    { "b",         "2M"    },
> > > +    { "g",         "120"   },
> > 
> > Why these values specifically? 
> 
> According to the default settings in nvenc (b = 2M) and vaapi encoder for h264 (g = 120).
> 
> >What happens if we leave them unset?
> 
> It would be 200kbps bitrate with gop size = 12.
> 
> There would be too much IDR frames with in small bitrate, which leads to
> a poor quality. 
> 
>  (I should have added such statements in the commit message)

I mean we could override the defaults to "-1" like is done for e.g.
x264, then we can distinguish between "the user specified the bitrate to
be <x>" vs. "the user did not specify anything about the target
bitrate".
And if the user did not specify the bitrate we could leave libopenh264
to choose bitrate on its own or use some other rate limiting mechanism
like constant QP or something CRF-like (don't know if it has that).
Fu, Linjie April 11, 2020, 9:37 a.m. UTC | #4
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton Khirnov
> Sent: Saturday, April 11, 2020 16:43
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> Quoting Fu, Linjie (2020-04-10 15:33:04)
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Anton Khirnov
> > > Sent: Friday, April 10, 2020 18:14
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> > > default gop size and bit rate
> > >
> > > Quoting Linjie Fu (2020-04-06 13:14:46)
> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > > ---
> > > >  libavcodec/libopenh264enc.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > > index c7ae5b1..3ff5be7 100644
> > > > --- a/libavcodec/libopenh264enc.c
> > > > +++ b/libavcodec/libopenh264enc.c
> > > > @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext
> > > *avctx, AVPacket *avpkt,
> > > >  }
> > > >
> > > >  static const AVCodecDefault svc_enc_defaults[] = {
> > > > +    { "b",         "2M"    },
> > > > +    { "g",         "120"   },
> > >
> > > Why these values specifically?
> >
> > According to the default settings in nvenc (b = 2M) and vaapi encoder for
> h264 (g = 120).
> >
> > >What happens if we leave them unset?
> >
> > It would be 200kbps bitrate with gop size = 12.
> >
> > There would be too much IDR frames with in small bitrate, which leads to
> > a poor quality.
> >
> >  (I should have added such statements in the commit message)
> 
> I mean we could override the defaults to "-1" like is done for e.g.
> x264, then we can distinguish between "the user specified the bitrate to
> be <x>" vs. "the user did not specify anything about the target
> bitrate".

Indeed, I noticed this while attempting to select rc_mode by avctx->bit_rate.
It makes sense to distinguish them, and I've changed it.

> And if the user did not specify the bitrate we could leave libopenh264
> to choose bitrate on its own or use some other rate limiting mechanism
> like constant QP or something CRF-like (don't know if it has that).

I'm afraid it doesn't have one if we leave the bitrate unset:
[OpenH264] this = 0x0x55a61f8dddd0, Error: Invalid bitrate settings in total configure, bitrate= 0

so I'll assign a default target bitrate to 2M.

- Linjie
diff mbox series

Patch

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index c7ae5b1..3ff5be7 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -335,6 +335,8 @@  static int svc_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
 }
 
 static const AVCodecDefault svc_enc_defaults[] = {
+    { "b",         "2M"    },
+    { "g",         "120"   },
     { "qmin",      "-1"    },
     { "qmax",      "-1"    },
     { NULL },