diff mbox

[FFmpeg-devel] lavc/qsvenc: set BRCParamMultiplier to aviod BRC overflow

Message ID 1547208578-26642-1-git-send-email-zhong.li@intel.com
State Accepted
Headers show

Commit Message

Zhong Li Jan. 11, 2019, 12:09 p.m. UTC 
Fix ticket #7663

Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/qsvenc.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

Comments

Carl Eugen Hoyos Jan. 11, 2019, 12:18 p.m. UTC | #1 
2019-01-11 13:09 GMT+01:00, Zhong Li <zhong.li@intel.com>:
> +    //libmfx BRC parameters are 16 bits thus maybe overflow, then
> BRCParamMultiplier is needed
> +    target_bitrate_kbps = avctx->bit_rate / 1000;
> +    max_bitrate_kbps = avctx->rc_max_rate / 1000;
> +    brc_param_multiplier = (FFMAX(target_bitrate_kbps, max_bitrate_kbps) +
> 0x10000) / 0x10000;

How will this behave if the user sets 100MBit?

Carl Eugen
Hendrik Leppkes Jan. 11, 2019, 1:25 p.m. UTC | #2 
On Fri, Jan 11, 2019 at 1:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-01-11 13:09 GMT+01:00, Zhong Li <zhong.li@intel.com>:
> > +    //libmfx BRC parameters are 16 bits thus maybe overflow, then
> > BRCParamMultiplier is needed
> > +    target_bitrate_kbps = avctx->bit_rate / 1000;
> > +    max_bitrate_kbps = avctx->rc_max_rate / 1000;
> > +    brc_param_multiplier = (FFMAX(target_bitrate_kbps, max_bitrate_kbps) +
> > 0x10000) / 0x10000;
>
> How will this behave if the user sets 100MBit?
>

Its really quite simple math. It'll cause the multipler to become 2,
effectively dividing all numbers by two, so instead of trying to pass
102400 kbps bitrate, it'll pass 51200 * 2 kbps bitrate, which fits
into the 16-bit value.

- Hendrik
Carl Eugen Hoyos Jan. 11, 2019, 1:45 p.m. UTC | #3 
2019-01-11 14:25 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Fri, Jan 11, 2019 at 1:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>> 2019-01-11 13:09 GMT+01:00, Zhong Li <zhong.li@intel.com>:
>> > +    //libmfx BRC parameters are 16 bits thus maybe overflow, then
>> > BRCParamMultiplier is needed
>> > +    target_bitrate_kbps = avctx->bit_rate / 1000;
>> > +    max_bitrate_kbps = avctx->rc_max_rate / 1000;
>> > +    brc_param_multiplier = (FFMAX(target_bitrate_kbps,
>> > max_bitrate_kbps) + 0x10000) / 0x10000;
>>
>> How will this behave if the user sets 100MBit?
>
> Its really quite simple math. It'll cause the multipler to become 2,
> effectively dividing all numbers by two, so instead of trying to pass
> 102400 kbps bitrate, it'll pass 51200 * 2 kbps bitrate, which fits
> into the 16-bit value.

Seems like a useful solution.

Is there no possibility that BufferSizeInKB or InitialDelayInKB overflow?

Carl Eugen
Zhong Li Jan. 11, 2019, 2:17 p.m. UTC | #4 
Good catch.Yes,that is possible but possibility is smaller than bitrate
since they are in Bytes. I got it too but finally ignored them to make
things simple. Let me send an update version soon.

Carl Eugen Hoyos <ceffmpeg@gmail.com> 于 2019年1月11日周五 下午9:45写道：

> 2019-01-11 14:25 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> > On Fri, Jan 11, 2019 at 1:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
> >>
> >> 2019-01-11 13:09 GMT+01:00, Zhong Li <zhong.li@intel.com>:
> >> > +    //libmfx BRC parameters are 16 bits thus maybe overflow, then
> >> > BRCParamMultiplier is needed
> >> > +    target_bitrate_kbps = avctx->bit_rate / 1000;
> >> > +    max_bitrate_kbps = avctx->rc_max_rate / 1000;
> >> > +    brc_param_multiplier = (FFMAX(target_bitrate_kbps,
> >> > max_bitrate_kbps) + 0x10000) / 0x10000;
> >>
> >> How will this behave if the user sets 100MBit?
> >
> > Its really quite simple math. It'll cause the multipler to become 2,
> > effectively dividing all numbers by two, so instead of trying to pass
> > 102400 kbps bitrate, it'll pass 51200 * 2 kbps bitrate, which fits
> > into the 16-bit value.
>
> Seems like a useful solution.
>
> Is there no possibility that BufferSizeInKB or InitialDelayInKB overflow?
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
diff mbox

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index e3b5a72..0e8229e 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -158,8 +158,8 @@  static void dump_video_param(AVCodecContext *avctx, QSVEncContext *q,
 #endif
         ) {
         av_log(avctx, AV_LOG_VERBOSE,
-               "BufferSizeInKB: %"PRIu16"; InitialDelayInKB: %"PRIu16"; TargetKbps: %"PRIu16"; MaxKbps: %"PRIu16"\n",
-               info->BufferSizeInKB, info->InitialDelayInKB, info->TargetKbps, info->MaxKbps);
+               "BufferSizeInKB: %"PRIu16"; InitialDelayInKB: %"PRIu16"; TargetKbps: %"PRIu16"; MaxKbps: %"PRIu16"; BRCParamMultiplier: %"PRIu16"\n",
+               info->BufferSizeInKB, info->InitialDelayInKB, info->TargetKbps, info->MaxKbps, info->BRCParamMultiplier);
     } else if (info->RateControlMethod == MFX_RATECONTROL_CQP) {
         av_log(avctx, AV_LOG_VERBOSE, "QPI: %"PRIu16"; QPP: %"PRIu16"; QPB: %"PRIu16"\n",
                info->QPI, info->QPP, info->QPB);
@@ -167,8 +167,8 @@  static void dump_video_param(AVCodecContext *avctx, QSVEncContext *q,
 #if QSV_HAVE_AVBR
     else if (info->RateControlMethod == MFX_RATECONTROL_AVBR) {
         av_log(avctx, AV_LOG_VERBOSE,
-               "TargetKbps: %"PRIu16"; Accuracy: %"PRIu16"; Convergence: %"PRIu16"\n",
-               info->TargetKbps, info->Accuracy, info->Convergence);
+               "TargetKbps: %"PRIu16"; Accuracy: %"PRIu16"; Convergence: %"PRIu16"; BRCParamMultiplier: %"PRIu16"\n",
+               info->TargetKbps, info->Accuracy, info->Convergence, info->BRCParamMultiplier);
     }
 #endif
 #if QSV_HAVE_LA
@@ -178,8 +178,8 @@  static void dump_video_param(AVCodecContext *avctx, QSVEncContext *q,
 #endif
              ) {
         av_log(avctx, AV_LOG_VERBOSE,
-               "TargetKbps: %"PRIu16"; LookAheadDepth: %"PRIu16"\n",
-               info->TargetKbps, co2->LookAheadDepth);
+               "TargetKbps: %"PRIu16"; LookAheadDepth: %"PRIu16"; BRCParamMultiplier: %"PRIu16"\n",
+               info->TargetKbps, co2->LookAheadDepth, info->BRCParamMultiplier);
     }
 #endif
 #if QSV_HAVE_ICQ
@@ -451,6 +451,7 @@  static int init_video_param(AVCodecContext *avctx, QSVEncContext *q)
                                    avctx->sw_pix_fmt : avctx->pix_fmt;
     const AVPixFmtDescriptor *desc;
     float quant;
+    int target_bitrate_kbps, max_bitrate_kbps, brc_param_multiplier;
     int ret;
     mfxVersion ver;
 
@@ -552,16 +553,22 @@  static int init_video_param(AVCodecContext *avctx, QSVEncContext *q)
     if (ret < 0)
         return ret;
 
+    //libmfx BRC parameters are 16 bits thus maybe overflow, then BRCParamMultiplier is needed
+    target_bitrate_kbps = avctx->bit_rate / 1000;
+    max_bitrate_kbps = avctx->rc_max_rate / 1000;
+    brc_param_multiplier = (FFMAX(target_bitrate_kbps, max_bitrate_kbps) + 0x10000) / 0x10000;
+
     switch (q->param.mfx.RateControlMethod) {
     case MFX_RATECONTROL_CBR:
     case MFX_RATECONTROL_VBR:
 #if QSV_HAVE_VCM
     case MFX_RATECONTROL_VCM:
 #endif
-        q->param.mfx.BufferSizeInKB   = avctx->rc_buffer_size / 8000;
-        q->param.mfx.InitialDelayInKB = avctx->rc_initial_buffer_occupancy / 1000;
-        q->param.mfx.TargetKbps       = avctx->bit_rate / 1000;
-        q->param.mfx.MaxKbps          = avctx->rc_max_rate / 1000;
+        q->param.mfx.BufferSizeInKB   = avctx->rc_buffer_size / 8000 / brc_param_multiplier;
+        q->param.mfx.InitialDelayInKB = avctx->rc_initial_buffer_occupancy / 1000 / brc_param_multiplier;
+        q->param.mfx.TargetKbps       = target_bitrate_kbps / brc_param_multiplier;
+        q->param.mfx.MaxKbps          = max_bitrate_kbps / brc_param_multiplier;
+        q->param.mfx.BRCParamMultiplier = brc_param_multiplier;
         break;
     case MFX_RATECONTROL_CQP:
         quant = avctx->global_quality / FF_QP2LAMBDA;
@@ -573,15 +580,17 @@  static int init_video_param(AVCodecContext *avctx, QSVEncContext *q)
         break;
 #if QSV_HAVE_AVBR
     case MFX_RATECONTROL_AVBR:
-        q->param.mfx.TargetKbps  = avctx->bit_rate / 1000;
+        q->param.mfx.TargetKbps  = target_bitrate_kbps / brc_param_multiplier;
         q->param.mfx.Convergence = q->avbr_convergence;
         q->param.mfx.Accuracy    = q->avbr_accuracy;
+        q->param.mfx.BRCParamMultiplier = brc_param_multiplier;
         break;
 #endif
 #if QSV_HAVE_LA
     case MFX_RATECONTROL_LA:
-        q->param.mfx.TargetKbps  = avctx->bit_rate / 1000;
+        q->param.mfx.TargetKbps  = target_bitrate_kbps / brc_param_multiplier;
         q->extco2.LookAheadDepth = q->look_ahead_depth;
+        q->param.mfx.BRCParamMultiplier = brc_param_multiplier;
         break;
 #if QSV_HAVE_ICQ
     case MFX_RATECONTROL_LA_ICQ:
@@ -726,7 +735,7 @@  static int qsv_retrieve_enc_jpeg_params(AVCodecContext *avctx, QSVEncContext *q)
         return ff_qsv_print_error(avctx, ret,
                                   "Error calling GetVideoParam");
 
-    q->packet_size = q->param.mfx.BufferSizeInKB * 1000;
+    q->packet_size = q->param.mfx.BufferSizeInKB * q->param.mfx.BRCParamMultiplier * 1000;
 
     // for qsv mjpeg the return value maybe 0 so alloc the buffer
     if (q->packet_size == 0)
@@ -779,7 +788,7 @@  static int qsv_retrieve_enc_params(AVCodecContext *avctx, QSVEncContext *q)
         return ff_qsv_print_error(avctx, ret,
                                   "Error calling GetVideoParam");
 
-    q->packet_size = q->param.mfx.BufferSizeInKB * 1000;
+    q->packet_size = q->param.mfx.BufferSizeInKB * q->param.mfx.BRCParamMultiplier * 1000;
 
     if (!extradata.SPSBufSize || (need_pps && !extradata.PPSBufSize)) {
         av_log(avctx, AV_LOG_ERROR, "No extradata returned from libmfx.\n");