diff mbox series

[FFmpeg-devel,v5,3/5] lavc/libopenh264enc: add bit rate control select support

Message ID 1588065363-7104-4-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series enhancement for libopenh264
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 28, 2020, 9:16 a.m. UTC
RC_BITRATE_MODE:
    set BITS_EXCEEDED to iCurrentBitsLevel and allows QP adjust
    in RcCalculatePictureQp().

RC_BUFFERBASED_MODE:
    use buffer status to adjust the video quality, introduced in
    release 1.2.

RC_TIMESTAMP_MODE:
    bit rate control based on timestamp, introduced in release 1.4.

Default to use RC_QUALITY_MODE.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/libopenh264enc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Martin Storsjö April 28, 2020, 10:30 a.m. UTC | #1
On Tue, 28 Apr 2020, Linjie Fu wrote:

> RC_BITRATE_MODE:
>    set BITS_EXCEEDED to iCurrentBitsLevel and allows QP adjust
>    in RcCalculatePictureQp().
>
> RC_BUFFERBASED_MODE:
>    use buffer status to adjust the video quality, introduced in
>    release 1.2.
>
> RC_TIMESTAMP_MODE:
>    bit rate control based on timestamp, introduced in release 1.4.
>
> Default to use RC_QUALITY_MODE.
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> libavcodec/libopenh264enc.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index 0951412..93d3de2 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -49,6 +49,9 @@ typedef struct SVCContext {
>     int skip_frames;
>     int skipped;
>     int cabac;
> +
> +    // rate control mode
> +    int rc_mode;
> } SVCContext;
> 
> #define OFFSET(x) offsetof(SVCContext, x)
> @@ -73,6 +76,18 @@ static const AVOption options[] = {
>     { "max_nal_size", "set maximum NAL size in bytes", OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
>     { "allow_skip_frames", "allow skipping frames to hit the target bitrate", OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
>     { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE },
> +
> +    { "rc_mode", "Select rate control mode", OFFSET(rc_mode), AV_OPT_TYPE_INT, { .i64 = RC_QUALITY_MODE }, RC_OFF_MODE, RC_TIMESTAMP_MODE, VE, "rc_mode" },
> +        { "off",       "bit rate control off",                                                 0, AV_OPT_TYPE_CONST, { .i64 = RC_OFF_MODE },         0, 0, VE, "rc_mode" },
> +        { "quality",   "quality mode",                                                         0, AV_OPT_TYPE_CONST, { .i64 = RC_QUALITY_MODE },     0, 0, VE, "rc_mode" },
> +        { "bitrate",   "bitrate mode",                                                         0, AV_OPT_TYPE_CONST, { .i64 = RC_BITRATE_MODE },     0, 0, VE, "rc_mode" },
> +#if OPENH264_VER_AT_LEAST(1, 2)
> +        { "buffer",    "using buffer status to adjust the video quality (no bitrate control)", 0, AV_OPT_TYPE_CONST, { .i64 = RC_BUFFERBASED_MODE }, 0, 0, VE, "rc_mode" },
> +#endif

We don't need checks for 1.2 - the initial version of openh264 that we 
support is 1.3, so we only need checks for 1.4 and newer.

> +#if OPENH264_VER_AT_LEAST(1, 4)
> +        { "timestamp", "bit rate control based on timestamp",                                  0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE, "rc_mode" },
> +#endif
> +
>     { NULL }
> };

This commit seems to lack something that actually sets the rc_mode value 
in the parameters struct though - wasn't that part of the previous version 
of the patch?

// Martin
Fu, Linjie April 28, 2020, 4:20 p.m. UTC | #2
> From: Martin Storsjö <martin@martin.st>
> Sent: Tuesday, April 28, 2020 18:31
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> rate control select support
> 
> On Tue, 28 Apr 2020, Linjie Fu wrote:
> 
> > RC_BITRATE_MODE:
> >    set BITS_EXCEEDED to iCurrentBitsLevel and allows QP adjust
> >    in RcCalculatePictureQp().
> >
> > RC_BUFFERBASED_MODE:
> >    use buffer status to adjust the video quality, introduced in
> >    release 1.2.
> >
> > RC_TIMESTAMP_MODE:
> >    bit rate control based on timestamp, introduced in release 1.4.
> >
> > Default to use RC_QUALITY_MODE.
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> > libavcodec/libopenh264enc.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 0951412..93d3de2 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -49,6 +49,9 @@ typedef struct SVCContext {
> >     int skip_frames;
> >     int skipped;
> >     int cabac;
> > +
> > +    // rate control mode
> > +    int rc_mode;
> > } SVCContext;
> >
> > #define OFFSET(x) offsetof(SVCContext, x)
> > @@ -73,6 +76,18 @@ static const AVOption options[] = {
> >     { "max_nal_size", "set maximum NAL size in bytes",
> OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
> >     { "allow_skip_frames", "allow skipping frames to hit the target bitrate",
> OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
> >     { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 },
> 0, 1, VE },
> > +
> > +    { "rc_mode", "Select rate control mode", OFFSET(rc_mode),
> AV_OPT_TYPE_INT, { .i64 = RC_QUALITY_MODE }, RC_OFF_MODE,
> RC_TIMESTAMP_MODE, VE, "rc_mode" },
> > +        { "off",       "bit rate control off",                                                 0,
> AV_OPT_TYPE_CONST, { .i64 = RC_OFF_MODE },         0, 0, VE, "rc_mode" },
> > +        { "quality",   "quality mode",                                                         0,
> AV_OPT_TYPE_CONST, { .i64 = RC_QUALITY_MODE },     0, 0, VE, "rc_mode" },
> > +        { "bitrate",   "bitrate mode",                                                         0,
> AV_OPT_TYPE_CONST, { .i64 = RC_BITRATE_MODE },     0, 0, VE, "rc_mode" },
> > +#if OPENH264_VER_AT_LEAST(1, 2)
> > +        { "buffer",    "using buffer status to adjust the video quality (no bitrate
> control)", 0, AV_OPT_TYPE_CONST, { .i64 = RC_BUFFERBASED_MODE }, 0, 0,
> VE, "rc_mode" },
> > +#endif
> 
> We don't need checks for 1.2 - the initial version of openh264 that we
> support is 1.3, so we only need checks for 1.4 and newer.

Ahh, didn't noticed this until checked the commit message of
8a3d9ca603f4d15ecaa9ca379cbaab4ecaec8ce4.

IMHO it seems to be better if we add this version check in the configure as well.
(Did a quick verification with version modified in pkgconfig to 1.1.0, configure is
still good for libopenh264 )

Will send another fix for this if no against.
 
> > +#if OPENH264_VER_AT_LEAST(1, 4)
> > +        { "timestamp", "bit rate control based on timestamp",
> 0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE,
> "rc_mode" },
> > +#endif
> > +
> >     { NULL }
> > };
> 
> This commit seems to lack something that actually sets the rc_mode value
> in the parameters struct though - wasn't that part of the previous version
> of the patch?
> 
Did you mean setting rc_mode through parameters like bit_rate/qp?
This patch enables explicitly setting the rc_mode as part of that logic. 

As to full enhancement, it's WIP and I'm still considering some parameters
like -qp to specify exact QP. (libopenh264 seems to only accept iMax/MinQp
in RC_QUALITY_MODE) For specific QP settings like -qp 26, we have to select
RC_OFF_MODE and specify QP by iDLayerQp in sSpatialLayers, which seems
a little bit confusing. (setting -qp 26 which leads to a non-quality mode)

- Linjie
Fu, Linjie April 29, 2020, 2:59 a.m. UTC | #3
> From: Martin Storsjö <martin@martin.st>
> Sent: Wednesday, April 29, 2020 03:18
> To: Fu, Linjie <linjie.fu@intel.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> rate control select support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö <martin@martin.st>
> >> Sent: Tuesday, April 28, 2020 18:31
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >> Cc: Fu, Linjie <linjie.fu@intel.com>
> >> Subject: Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> >> rate control select support
> >>
> >> On Tue, 28 Apr 2020, Linjie Fu wrote:
> >>
> >> We don't need checks for 1.2 - the initial version of openh264 that we
> >> support is 1.3, so we only need checks for 1.4 and newer.
> >
> > Ahh, didn't noticed this until checked the commit message of
> > 8a3d9ca603f4d15ecaa9ca379cbaab4ecaec8ce4.
> >
> > IMHO it seems to be better if we add this version check in the configure as
> well.
> > (Did a quick verification with version modified in pkgconfig to 1.1.0,
> configure is
> > still good for libopenh264 )
> 
> We don't need an explicit version check for 1.3 in configure - we require
> that WelsGetCodecVersion can be found in the configure check, and that
> function was added in 1.3 (explicitly for the purpose so that we can check
> that the version that we have linked is the same as we compiled against).
> 

Checked and confirmed that you are right, WelsGetCodecVersion is enough
for  1.3.0 version check, thanks for elaborations.

> >
> >>> +#if OPENH264_VER_AT_LEAST(1, 4)
> >>> +        { "timestamp", "bit rate control based on timestamp",
> >> 0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE,
> >> "rc_mode" },
> >>> +#endif
> >>> +
> >>>     { NULL }
> >>> };
> >>
> >> This commit seems to lack something that actually sets the rc_mode value
> >> in the parameters struct though - wasn't that part of the previous version
> >> of the patch?
> >>
> > Did you mean setting rc_mode through parameters like bit_rate/qp?
> > This patch enables explicitly setting the rc_mode as part of that logic.
> 
> No, I didn't mean that.
> 
> The previous version of this patch had the following change:
> 
> -    param.iRCMode                    = RC_QUALITY_MODE;
> +    param.iRCMode                    = s->rc_mode;
> 
> Now I don't find that part any longer in this patch - and without that
> change, the rest of the commit is pretty pointless, no?

it's somehow missed while rebasing the set, thanks.

- Linjie
diff mbox series

Patch

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index 0951412..93d3de2 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -49,6 +49,9 @@  typedef struct SVCContext {
     int skip_frames;
     int skipped;
     int cabac;
+
+    // rate control mode
+    int rc_mode;
 } SVCContext;
 
 #define OFFSET(x) offsetof(SVCContext, x)
@@ -73,6 +76,18 @@  static const AVOption options[] = {
     { "max_nal_size", "set maximum NAL size in bytes", OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
     { "allow_skip_frames", "allow skipping frames to hit the target bitrate", OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
     { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE },
+
+    { "rc_mode", "Select rate control mode", OFFSET(rc_mode), AV_OPT_TYPE_INT, { .i64 = RC_QUALITY_MODE }, RC_OFF_MODE, RC_TIMESTAMP_MODE, VE, "rc_mode" },
+        { "off",       "bit rate control off",                                                 0, AV_OPT_TYPE_CONST, { .i64 = RC_OFF_MODE },         0, 0, VE, "rc_mode" },
+        { "quality",   "quality mode",                                                         0, AV_OPT_TYPE_CONST, { .i64 = RC_QUALITY_MODE },     0, 0, VE, "rc_mode" },
+        { "bitrate",   "bitrate mode",                                                         0, AV_OPT_TYPE_CONST, { .i64 = RC_BITRATE_MODE },     0, 0, VE, "rc_mode" },
+#if OPENH264_VER_AT_LEAST(1, 2)
+        { "buffer",    "using buffer status to adjust the video quality (no bitrate control)", 0, AV_OPT_TYPE_CONST, { .i64 = RC_BUFFERBASED_MODE }, 0, 0, VE, "rc_mode" },
+#endif
+#if OPENH264_VER_AT_LEAST(1, 4)
+        { "timestamp", "bit rate control based on timestamp",                                  0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE, "rc_mode" },
+#endif
+
     { NULL }
 };