| Message ID | 20180607234331.32139-17-sw@jkqxz.net |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jun 8, 2018 at 7:45 AM Mark Thompson <sw@jkqxz.net> wrote: > > Query which modes are supported and select between VBR and CBR based > on that - this removes all of the codec-specific rate control mode > selection code. > --- > doc/encoders.texi | 2 - > libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++------------ > libavcodec/vaapi_encode.h | 6 +- > libavcodec/vaapi_encode_h264.c | 18 +---- > libavcodec/vaapi_encode_h265.c | 14 +--- > libavcodec/vaapi_encode_mjpeg.c | 3 +- > libavcodec/vaapi_encode_mpeg2.c | 9 +-- > libavcodec/vaapi_encode_vp8.c | 13 +-- > libavcodec/vaapi_encode_vp9.c | 13 +-- > 9 files changed, 137 insertions(+), 114 deletions(-) > > diff --git a/doc/encoders.texi b/doc/encoders.texi > index 62a1509a96..0c0a307987 100644 > --- a/doc/encoders.texi > +++ b/doc/encoders.texi > @@ -2643,8 +2643,6 @@ Always encodes using the standard quantisation and huffman tables - > @item mpeg2_vaapi > @option{profile} and @option{level} set the value of @emph{profile_and_level_indication}. > > -No rate control is supported. > - > @item vp8_vaapi > B-frames are not supported. > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > index f4c063734c..5de5483454 100644 > --- a/libavcodec/vaapi_encode.c > +++ b/libavcodec/vaapi_encode.c > @@ -1217,7 +1217,6 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) > int i; > > VAConfigAttrib attr[] = { > - { VAConfigAttribRateControl }, > { VAConfigAttribEncMaxRefFrames }, > { VAConfigAttribEncPackedHeaders }, > }; > @@ -1241,32 +1240,6 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) > continue; > } > switch (attr[i].type) { > - case VAConfigAttribRateControl: > - // Hack for backward compatibility: CBR was the only > - // usable RC mode for a long time, so old drivers will > - // only have it. Normal default options may now choose > - // VBR and then fail, however, so override it here with > - // CBR if that is the only supported mode. > - if (ctx->va_rc_mode == VA_RC_VBR && > - !(attr[i].value & VA_RC_VBR) && > - (attr[i].value & VA_RC_CBR)) { > - av_log(avctx, AV_LOG_WARNING, "VBR rate control is " > - "not supported with this driver version; " > - "using CBR instead.\n"); > - ctx->va_rc_mode = VA_RC_CBR; > - } > - if (!(ctx->va_rc_mode & attr[i].value)) { > - av_log(avctx, AV_LOG_ERROR, "Rate control mode %#x " > - "is not supported (mask: %#x).\n", > - ctx->va_rc_mode, attr[i].value); > - return AVERROR(EINVAL); > - } > - ctx->config_attributes[ctx->nb_config_attributes++] = > - (VAConfigAttrib) { > - .type = VAConfigAttribRateControl, > - .value = ctx->va_rc_mode, > - }; > - break; > case VAConfigAttribEncMaxRefFrames: > { > unsigned int ref_l0 = attr[i].value & 0xffff; > @@ -1313,44 +1286,144 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) > static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) > { > VAAPIEncodeContext *ctx = avctx->priv_data; > - int rc_bits_per_second; > - int rc_target_percentage; > - int rc_window_size; > - int hrd_buffer_size; > - int hrd_initial_buffer_fullness; > + int64_t rc_bits_per_second; > + int rc_target_percentage; > + int rc_window_size; > + int64_t hrd_buffer_size; > + int64_t hrd_initial_buffer_fullness; > int fr_num, fr_den; > + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; > + VAStatus vas; > + > + vas = vaGetConfigAttributes(ctx->hwctx->display, > + ctx->va_profile, ctx->va_entrypoint, > + &rc_attr, 1); > + if (vas != VA_STATUS_SUCCESS) { > + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " > + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); > + return AVERROR_EXTERNAL; > + } > > - if (avctx->bit_rate > INT32_MAX) { > - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " > - "higher is not supported.\n"); > + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { > + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " > + "supported rate control modes: assuming constant-quality.\n"); > + ctx->va_rc_mode = VA_RC_CQP; > + return 0; > + } > + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > + avctx->bit_rate <= 0) { > + if (rc_attr.value & VA_RC_CQP) { > + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality mode.\n"); > + ctx->va_rc_mode = VA_RC_CQP; > + return 0; > + } else { > + av_log(avctx, AV_LOG_ERROR, "Driver does not support " > + "constant-quality mode (%#x).\n", rc_attr.value); > + return AVERROR(EINVAL); > + } > + } > + > + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { > + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " > + "bitrate-targetted rate control modes.\n"); > return AVERROR(EINVAL); > } > > if (avctx->rc_buffer_size) > hrd_buffer_size = avctx->rc_buffer_size; > + else if (avctx->rc_max_rate > 0) > + hrd_buffer_size = avctx->rc_max_rate; > else > hrd_buffer_size = avctx->bit_rate; > - if (avctx->rc_initial_buffer_occupancy) > + if (avctx->rc_initial_buffer_occupancy) { > + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { > + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " > + "must have initial buffer size (%d) < " > + "buffer size (%"PRId64").\n", > + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); > + return AVERROR(EINVAL); > + } > hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; > - else > + } else { > hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; > + } > + > + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { > + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have " > + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", > + avctx->bit_rate, avctx->rc_max_rate); > + return AVERROR(EINVAL); > + } > + > + if (avctx->rc_max_rate > avctx->bit_rate) { > + if (!(rc_attr.value & VA_RC_VBR)) { > + av_log(avctx, AV_LOG_WARNING, "Driver does not support " > + "VBR mode (%#x), using CBR mode instead.\n", > + rc_attr.value); > + ctx->va_rc_mode = VA_RC_CBR; > + } else { > + ctx->va_rc_mode = VA_RC_VBR; > + } > + > + rc_bits_per_second = avctx->rc_max_rate; > + rc_target_percentage = (avctx->bit_rate * 100) / avctx->rc_max_rate; > + > + } else if (avctx->rc_max_rate == avctx->bit_rate) { > + if (!(rc_attr.value & VA_RC_CBR)) { > + av_log(avctx, AV_LOG_WARNING, "Driver does not support " > + "CBR mode (%#x), using VBR mode instead.\n", > + rc_attr.value); > + ctx->va_rc_mode = VA_RC_VBR; > + } else { > + ctx->va_rc_mode = VA_RC_CBR; > + } > > - if (ctx->va_rc_mode == VA_RC_CBR) { > rc_bits_per_second = avctx->bit_rate; > rc_target_percentage = 100; > - rc_window_size = 1000; > + > } else { > - if (avctx->rc_max_rate < avctx->bit_rate) { > - // Max rate is unset or invalid, just use the normal bitrate. > + if (rc_attr.value & VA_RC_VBR) { > + ctx->va_rc_mode = VA_RC_VBR; > + > + // We only have a target bitrate, but VAAPI requires that a > + // maximum rate be supplied as well. Since the user has > + // offered no particular constraint, arbitrarily pick a > + // maximum rate of double the target rate. > + rc_bits_per_second = 2 * avctx->bit_rate; > + rc_target_percentage = 50; > + } else { > + ctx->va_rc_mode = VA_RC_CBR; > + > rc_bits_per_second = avctx->bit_rate; > rc_target_percentage = 100; > - } else { > - rc_bits_per_second = avctx->rc_max_rate; > - rc_target_percentage = (avctx->bit_rate * 100) / rc_bits_per_second; > } > - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; > } > > + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; > + > + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " > + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", > + rc_target_percentage, rc_bits_per_second, rc_window_size); > + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " > + "initial fullness %"PRId64" bits.\n", > + hrd_buffer_size, hrd_initial_buffer_fullness); > + > + if (rc_bits_per_second > UINT32_MAX || > + hrd_buffer_size > UINT32_MAX || > + hrd_initial_buffer_fullness > UINT32_MAX) { > + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " > + "greater are not supported by VAAPI.\n"); > + return AVERROR(EINVAL); > + } > + Use a assert0() maybe more pithily > + ctx->va_bit_rate = rc_bits_per_second; > + > + ctx->config_attributes[ctx->nb_config_attributes++] = > + (VAConfigAttrib) { > + .type = VAConfigAttribRateControl, > + .value = ctx->va_rc_mode, > + }; > + > ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl; > ctx->rc_params.rc = (VAEncMiscParameterRateControl) { > .bits_per_second = rc_bits_per_second, > @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) > if (err < 0) > goto fail; > > + err = vaapi_encode_init_rate_control(avctx); > + if (err < 0) > + goto fail; > + > err = vaapi_encode_config_attributes(avctx); > if (err < 0) > goto fail; > @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) > goto fail; > } > > - if (ctx->va_rc_mode & ~VA_RC_CQP) { > - err = vaapi_encode_init_rate_control(avctx); > - if (err < 0) > - goto fail; > - } > - > if (ctx->codec->configure) { > err = ctx->codec->configure(avctx); > if (err < 0) > diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h > index b8f2ed6d21..6fac4781c3 100644 > --- a/libavcodec/vaapi_encode.h > +++ b/libavcodec/vaapi_encode.h > @@ -116,8 +116,6 @@ typedef struct VAAPIEncodeContext { > // Use low power encoding mode. > int low_power; > > - // Rate control mode. > - unsigned int va_rc_mode; > // Supported packed headers (initially the desired set, modified > // later to what is actually supported). > unsigned int va_packed_headers; > @@ -138,6 +136,10 @@ typedef struct VAAPIEncodeContext { > VAProfile va_profile; > // Encoding entrypoint (VAEntryoint*). > VAEntrypoint va_entrypoint; > + // Rate control mode. > + unsigned int va_rc_mode; > + // Bitrate for codec-specific encoder parameters. > + unsigned int va_bit_rate; > > // Configuration attributes to use when creating va_config. > VAConfigAttrib config_attributes[MAX_CONFIG_ATTRIBUTES]; > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > index df6f39a946..1e6eb2e39b 100644 > --- a/libavcodec/vaapi_encode_h264.c > +++ b/libavcodec/vaapi_encode_h264.c > @@ -425,9 +425,9 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > // Try to scale these to a sensible range so that the > // golomb encode of the value is not overlong. > hrd->bit_rate_scale = > - av_clip_uintp2(av_log2(avctx->bit_rate) - 15 - 6, 4); > + av_clip_uintp2(av_log2(ctx->va_bit_rate) - 15 - 6, 4); > hrd->bit_rate_value_minus1[0] = > - (avctx->bit_rate >> hrd->bit_rate_scale + 6) - 1; > + (ctx->va_bit_rate >> hrd->bit_rate_scale + 6) - 1; > > hrd->cpb_size_scale = > av_clip_uintp2(av_log2(ctx->hrd_params.hrd.buffer_size) - 15 - 4, 4); > @@ -497,7 +497,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > .intra_idr_period = avctx->gop_size, > .ip_period = ctx->b_per_p + 1, > > - .bits_per_second = avctx->bit_rate, > + .bits_per_second = ctx->va_bit_rate, > .max_num_ref_frames = sps->max_num_ref_frames, > .picture_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1, > .picture_height_in_mbs = sps->pic_height_in_map_units_minus1 + 1, > @@ -823,10 +823,6 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx) > priv->fixed_qp_p = 26; > priv->fixed_qp_b = 26; > > - av_log(avctx, AV_LOG_DEBUG, "Using %s-bitrate = %"PRId64" bps.\n", > - ctx->va_rc_mode == VA_RC_CBR ? "constant" : "variable", > - avctx->bit_rate); > - > } else { > av_assert0(0 && "Invalid RC mode."); > } > @@ -934,14 +930,6 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx) > return AVERROR_PATCHWELCOME; > } > > - if (avctx->bit_rate > 0) { > - if (avctx->rc_max_rate == avctx->bit_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else > - ctx->va_rc_mode = VA_RC_CQP; > - > ctx->va_packed_headers = > VA_ENC_PACKED_HEADER_SEQUENCE | // SPS and PPS. > VA_ENC_PACKED_HEADER_SLICE | // Slice headers. > diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c > index b8b66b87cb..b296919b37 100644 > --- a/libavcodec/vaapi_encode_h265.c > +++ b/libavcodec/vaapi_encode_h265.c > @@ -512,7 +512,7 @@ static int vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx) > .intra_period = avctx->gop_size, > .intra_idr_period = avctx->gop_size, > .ip_period = ctx->b_per_p + 1, > - .bits_per_second = avctx->bit_rate, > + .bits_per_second = ctx->va_bit_rate, > > .pic_width_in_luma_samples = sps->pic_width_in_luma_samples, > .pic_height_in_luma_samples = sps->pic_height_in_luma_samples, > @@ -1014,10 +1014,6 @@ static av_cold int vaapi_encode_h265_configure(AVCodecContext *avctx) > priv->fixed_qp_p = 30; > priv->fixed_qp_b = 30; > > - av_log(avctx, AV_LOG_DEBUG, "Using %s-bitrate = %"PRId64" bps.\n", > - ctx->va_rc_mode == VA_RC_CBR ? "constant" : "variable", > - avctx->bit_rate); > - > } else { > av_assert0(0 && "Invalid RC mode."); > } > @@ -1068,14 +1064,6 @@ static av_cold int vaapi_encode_h265_init(AVCodecContext *avctx) > if (avctx->level == FF_LEVEL_UNKNOWN) > avctx->level = priv->level; > > - if (avctx->bit_rate > 0) { > - if (avctx->rc_max_rate == avctx->bit_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else > - ctx->va_rc_mode = VA_RC_CQP; > - > ctx->va_packed_headers = > VA_ENC_PACKED_HEADER_SEQUENCE | // VPS, SPS and PPS. > VA_ENC_PACKED_HEADER_SLICE | // Slice headers. > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c > index b328beaa09..67ac2fba96 100644 > --- a/libavcodec/vaapi_encode_mjpeg.c > +++ b/libavcodec/vaapi_encode_mjpeg.c > @@ -388,8 +388,6 @@ static av_cold int vaapi_encode_mjpeg_init(AVCodecContext *avctx) > > ctx->codec = &vaapi_encode_type_mjpeg; > > - ctx->va_rc_mode = VA_RC_CQP; > - > // The JPEG image header - see note above. > ctx->va_packed_headers = > VA_ENC_PACKED_HEADER_RAW_DATA; > @@ -402,6 +400,7 @@ static av_cold int vaapi_encode_mjpeg_init(AVCodecContext *avctx) > > static const AVCodecDefault vaapi_encode_mjpeg_defaults[] = { > { "global_quality", "80" }, > + { "b", "0" }, > { NULL }, > }; > > diff --git a/libavcodec/vaapi_encode_mpeg2.c b/libavcodec/vaapi_encode_mpeg2.c > index db79d72115..ff86b8817e 100644 > --- a/libavcodec/vaapi_encode_mpeg2.c > +++ b/libavcodec/vaapi_encode_mpeg2.c > @@ -188,8 +188,8 @@ static int vaapi_encode_mpeg2_init_sequence_params(AVCodecContext *avctx) > memset(pce, 0, sizeof(*pce)); > > > - if (avctx->bit_rate > 0) { > - priv->bit_rate = (avctx->bit_rate + 399) / 400; > + if (ctx->va_bit_rate > 0) { > + priv->bit_rate = (ctx->va_bit_rate + 399) / 400; > } else { > // Unknown (not a bitrate-targetting mode), so just use the > // highest value. > @@ -361,7 +361,7 @@ static int vaapi_encode_mpeg2_init_sequence_params(AVCodecContext *avctx) > .picture_width = avctx->width, > .picture_height = avctx->height, > > - .bits_per_second = avctx->bit_rate, > + .bits_per_second = ctx->va_bit_rate, > .frame_rate = av_q2d(priv->frame_rate), > .aspect_ratio_information = sh->aspect_ratio_information, > .vbv_buffer_size = priv->vbv_buffer_size, > @@ -615,8 +615,6 @@ static av_cold int vaapi_encode_mpeg2_init(AVCodecContext *avctx) > return AVERROR(EINVAL); > } > > - ctx->va_rc_mode = VA_RC_CQP; > - > ctx->va_packed_headers = VA_ENC_PACKED_HEADER_SEQUENCE | > VA_ENC_PACKED_HEADER_PICTURE; > > @@ -666,6 +664,7 @@ static const AVOption vaapi_encode_mpeg2_options[] = { > }; > > static const AVCodecDefault vaapi_encode_mpeg2_defaults[] = { > + { "b", "0" }, > { "bf", "1" }, > { "g", "120" }, > { "i_qfactor", "1" }, > diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c > index 9588826bfb..40871a6bbf 100644 > --- a/libavcodec/vaapi_encode_vp8.c > +++ b/libavcodec/vaapi_encode_vp8.c > @@ -65,7 +65,7 @@ static int vaapi_encode_vp8_init_sequence_params(AVCodecContext *avctx) > vseq->kf_auto = 0; > > if (!(ctx->va_rc_mode & VA_RC_CQP)) { > - vseq->bits_per_second = avctx->bit_rate; > + vseq->bits_per_second = ctx->va_bit_rate; > vseq->intra_period = avctx->gop_size; > } > > @@ -205,17 +205,6 @@ static av_cold int vaapi_encode_vp8_init(AVCodecContext *avctx) > > ctx->codec = &vaapi_encode_type_vp8; > > - if (avctx->flags & AV_CODEC_FLAG_QSCALE) { > - ctx->va_rc_mode = VA_RC_CQP; > - } else if (avctx->bit_rate > 0) { > - if (avctx->rc_max_rate == avctx->bit_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else { > - ctx->va_rc_mode = VA_RC_CQP; > - } > - > // Packed headers are not currently supported. > ctx->va_packed_headers = 0; > > diff --git a/libavcodec/vaapi_encode_vp9.c b/libavcodec/vaapi_encode_vp9.c > index 4d7cec0520..e400bc8b79 100644 > --- a/libavcodec/vaapi_encode_vp9.c > +++ b/libavcodec/vaapi_encode_vp9.c > @@ -71,7 +71,7 @@ static int vaapi_encode_vp9_init_sequence_params(AVCodecContext *avctx) > vseq->kf_auto = 0; > > if (!(ctx->va_rc_mode & VA_RC_CQP)) { > - vseq->bits_per_second = avctx->bit_rate; > + vseq->bits_per_second = ctx->va_bit_rate; > vseq->intra_period = avctx->gop_size; > } > > @@ -227,17 +227,6 @@ static av_cold int vaapi_encode_vp9_init(AVCodecContext *avctx) > > ctx->codec = &vaapi_encode_type_vp9; > > - if (avctx->flags & AV_CODEC_FLAG_QSCALE) { > - ctx->va_rc_mode = VA_RC_CQP; > - } else if (avctx->bit_rate > 0) { > - if (avctx->bit_rate == avctx->rc_max_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else { > - ctx->va_rc_mode = VA_RC_CQP; > - } > - > // Packed headers are not currently supported. > ctx->va_packed_headers = 0; > > -- > 2.16.3 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: > Query which modes are supported and select between VBR and CBR based > on that - this removes all of the codec-specific rate control mode > selection code. > --- > doc/encoders.texi | 2 - > libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++----------- > - > libavcodec/vaapi_encode.h | 6 +- > libavcodec/vaapi_encode_h264.c | 18 +---- > libavcodec/vaapi_encode_h265.c | 14 +--- > libavcodec/vaapi_encode_mjpeg.c | 3 +- > libavcodec/vaapi_encode_mpeg2.c | 9 +-- > libavcodec/vaapi_encode_vp8.c | 13 +-- > libavcodec/vaapi_encode_vp9.c | 13 +-- > 9 files changed, 137 insertions(+), 114 deletions(-) > > diff --git a/doc/encoders.texi b/doc/encoders.texi > index 62a1509a96..0c0a307987 100644 > --- a/doc/encoders.texi > +++ b/doc/encoders.texi > @@ -2643,8 +2643,6 @@ Always encodes using the standard quantisation and > huffman tables - > @item mpeg2_vaapi > @option{profile} and @option{level} set the value of > @emph{profile_and_level_indication}. > > -No rate control is supported. > - > @item vp8_vaapi > B-frames are not supported. > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > index f4c063734c..5de5483454 100644 > --- a/libavcodec/vaapi_encode.c > +++ b/libavcodec/vaapi_encode.c > @@ -1217,7 +1217,6 @@ static av_cold int > vaapi_encode_config_attributes(AVCodecContext *avctx) > int i; > > VAConfigAttrib attr[] = { > - { VAConfigAttribRateControl }, > { VAConfigAttribEncMaxRefFrames }, > { VAConfigAttribEncPackedHeaders }, > }; > @@ -1241,32 +1240,6 @@ static av_cold int > vaapi_encode_config_attributes(AVCodecContext *avctx) > continue; > } > switch (attr[i].type) { > - case VAConfigAttribRateControl: > - // Hack for backward compatibility: CBR was the only > - // usable RC mode for a long time, so old drivers will > - // only have it. Normal default options may now choose > - // VBR and then fail, however, so override it here with > - // CBR if that is the only supported mode. > - if (ctx->va_rc_mode == VA_RC_VBR && > - !(attr[i].value & VA_RC_VBR) && > - (attr[i].value & VA_RC_CBR)) { > - av_log(avctx, AV_LOG_WARNING, "VBR rate control is " > - "not supported with this driver version; " > - "using CBR instead.\n"); > - ctx->va_rc_mode = VA_RC_CBR; > - } > - if (!(ctx->va_rc_mode & attr[i].value)) { > - av_log(avctx, AV_LOG_ERROR, "Rate control mode %#x " > - "is not supported (mask: %#x).\n", > - ctx->va_rc_mode, attr[i].value); > - return AVERROR(EINVAL); > - } > - ctx->config_attributes[ctx->nb_config_attributes++] = > - (VAConfigAttrib) { > - .type = VAConfigAttribRateControl, > - .value = ctx->va_rc_mode, > - }; > - break; > case VAConfigAttribEncMaxRefFrames: > { > unsigned int ref_l0 = attr[i].value & 0xffff; > @@ -1313,44 +1286,144 @@ static av_cold int > vaapi_encode_config_attributes(AVCodecContext *avctx) > static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) > { > VAAPIEncodeContext *ctx = avctx->priv_data; > - int rc_bits_per_second; > - int rc_target_percentage; > - int rc_window_size; > - int hrd_buffer_size; > - int hrd_initial_buffer_fullness; > + int64_t rc_bits_per_second; > + int rc_target_percentage; > + int rc_window_size; > + int64_t hrd_buffer_size; > + int64_t hrd_initial_buffer_fullness; > int fr_num, fr_den; > + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; > + VAStatus vas; > + > + vas = vaGetConfigAttributes(ctx->hwctx->display, > + ctx->va_profile, ctx->va_entrypoint, > + &rc_attr, 1); > + if (vas != VA_STATUS_SUCCESS) { > + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " > + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); > + return AVERROR_EXTERNAL; > + } > > - if (avctx->bit_rate > INT32_MAX) { > - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " > - "higher is not supported.\n"); > + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { > + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " > + "supported rate control modes: assuming constant-quality.\n"); How about logging it as warning message? > + ctx->va_rc_mode = VA_RC_CQP; > + return 0; > + } > + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > + avctx->bit_rate <= 0) { > + if (rc_attr.value & VA_RC_CQP) { > + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality mode.\n"); > + ctx->va_rc_mode = VA_RC_CQP; > + return 0; > + } else { > + av_log(avctx, AV_LOG_ERROR, "Driver does not support " > + "constant-quality mode (%#x).\n", rc_attr.value); > + return AVERROR(EINVAL); > + } > + } > + > + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { > + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " > + "bitrate-targetted rate control modes.\n"); > return AVERROR(EINVAL); > } > > if (avctx->rc_buffer_size) > hrd_buffer_size = avctx->rc_buffer_size; > + else if (avctx->rc_max_rate > 0) > + hrd_buffer_size = avctx->rc_max_rate; > else > hrd_buffer_size = avctx->bit_rate; > - if (avctx->rc_initial_buffer_occupancy) > + if (avctx->rc_initial_buffer_occupancy) { > + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { > + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " > + "must have initial buffer size (%d) < " > + "buffer size (%"PRId64").\n", > + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); > + return AVERROR(EINVAL); > + } > hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; > - else > + } else { > hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; > + } > + > + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { > + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have " > + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", > + avctx->bit_rate, avctx->rc_max_rate); > + return AVERROR(EINVAL); > + } > + > + if (avctx->rc_max_rate > avctx->bit_rate) { > + if (!(rc_attr.value & VA_RC_VBR)) { > + av_log(avctx, AV_LOG_WARNING, "Driver does not support " > + "VBR mode (%#x), using CBR mode instead.\n", > + rc_attr.value); > + ctx->va_rc_mode = VA_RC_CBR; > + } else { > + ctx->va_rc_mode = VA_RC_VBR; > + } > + > + rc_bits_per_second = avctx->rc_max_rate; > + rc_target_percentage = (avctx->bit_rate * 100) / avctx->rc_max_rate; I think rc_target_percentage should be 100 for CBR case. > + > + } else if (avctx->rc_max_rate == avctx->bit_rate) { > + if (!(rc_attr.value & VA_RC_CBR)) { > + av_log(avctx, AV_LOG_WARNING, "Driver does not support " > + "CBR mode (%#x), using VBR mode instead.\n", > + rc_attr.value); > + ctx->va_rc_mode = VA_RC_VBR; > + } else { > + ctx->va_rc_mode = VA_RC_CBR; > + } > > - if (ctx->va_rc_mode == VA_RC_CBR) { > rc_bits_per_second = avctx->bit_rate; > rc_target_percentage = 100; > - rc_window_size = 1000; > + > } else { > - if (avctx->rc_max_rate < avctx->bit_rate) { > - // Max rate is unset or invalid, just use the normal bitrate. > + if (rc_attr.value & VA_RC_VBR) { > + ctx->va_rc_mode = VA_RC_VBR; Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR is supported by driver? > + > + // We only have a target bitrate, but VAAPI requires that a > + // maximum rate be supplied as well. Since the user has > + // offered no particular constraint, arbitrarily pick a > + // maximum rate of double the target rate. > + rc_bits_per_second = 2 * avctx->bit_rate; > + rc_target_percentage = 50; > + } else { > + ctx->va_rc_mode = VA_RC_CBR; > + > rc_bits_per_second = avctx->bit_rate; > rc_target_percentage = 100; > - } else { > - rc_bits_per_second = avctx->rc_max_rate; > - rc_target_percentage = (avctx->bit_rate * 100) / > rc_bits_per_second; > } > - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; > } > > + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; > + > + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " > + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", > + rc_target_percentage, rc_bits_per_second, rc_window_size); > + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " > + "initial fullness %"PRId64" bits.\n", > + hrd_buffer_size, hrd_initial_buffer_fullness); > + > + if (rc_bits_per_second > UINT32_MAX || > + hrd_buffer_size > UINT32_MAX || > + hrd_initial_buffer_fullness > UINT32_MAX) { > + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " > + "greater are not supported by VAAPI.\n"); > + return AVERROR(EINVAL); > + } > + > + ctx->va_bit_rate = rc_bits_per_second; > + > + ctx->config_attributes[ctx->nb_config_attributes++] = > + (VAConfigAttrib) { > + .type = VAConfigAttribRateControl, > + .value = ctx->va_rc_mode, > + }; > + > ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl; > ctx->rc_params.rc = (VAEncMiscParameterRateControl) { > .bits_per_second = rc_bits_per_second, > @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) > if (err < 0) > goto fail; > > + err = vaapi_encode_init_rate_control(avctx); > + if (err < 0) > + goto fail; > + > err = vaapi_encode_config_attributes(avctx); > if (err < 0) > goto fail; > @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) > goto fail; > } > > - if (ctx->va_rc_mode & ~VA_RC_CQP) { > - err = vaapi_encode_init_rate_control(avctx); > - if (err < 0) > - goto fail; > - } > - > if (ctx->codec->configure) { > err = ctx->codec->configure(avctx); > if (err < 0) > diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h > index b8f2ed6d21..6fac4781c3 100644 > --- a/libavcodec/vaapi_encode.h > +++ b/libavcodec/vaapi_encode.h > @@ -116,8 +116,6 @@ typedef struct VAAPIEncodeContext { > // Use low power encoding mode. > int low_power; > > - // Rate control mode. > - unsigned int va_rc_mode; > // Supported packed headers (initially the desired set, modified > // later to what is actually supported). > unsigned int va_packed_headers; > @@ -138,6 +136,10 @@ typedef struct VAAPIEncodeContext { > VAProfile va_profile; > // Encoding entrypoint (VAEntryoint*). > VAEntrypoint va_entrypoint; > + // Rate control mode. > + unsigned int va_rc_mode; > + // Bitrate for codec-specific encoder parameters. > + unsigned int va_bit_rate; > > // Configuration attributes to use when creating va_config. > VAConfigAttrib config_attributes[MAX_CONFIG_ATTRIBUTES]; > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > index df6f39a946..1e6eb2e39b 100644 > --- a/libavcodec/vaapi_encode_h264.c > +++ b/libavcodec/vaapi_encode_h264.c > @@ -425,9 +425,9 @@ static int > vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > // Try to scale these to a sensible range so that the > // golomb encode of the value is not overlong. > hrd->bit_rate_scale = > - av_clip_uintp2(av_log2(avctx->bit_rate) - 15 - 6, 4); > + av_clip_uintp2(av_log2(ctx->va_bit_rate) - 15 - 6, 4); > hrd->bit_rate_value_minus1[0] = > - (avctx->bit_rate >> hrd->bit_rate_scale + 6) - 1; > + (ctx->va_bit_rate >> hrd->bit_rate_scale + 6) - 1; > > hrd->cpb_size_scale = > av_clip_uintp2(av_log2(ctx->hrd_params.hrd.buffer_size) - 15 - 4, > 4); > @@ -497,7 +497,7 @@ static int > vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > .intra_idr_period = avctx->gop_size, > .ip_period = ctx->b_per_p + 1, > > - .bits_per_second = avctx->bit_rate, > + .bits_per_second = ctx->va_bit_rate, > .max_num_ref_frames = sps->max_num_ref_frames, > .picture_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1, > .picture_height_in_mbs = sps->pic_height_in_map_units_minus1 + 1, > @@ -823,10 +823,6 @@ static av_cold int > vaapi_encode_h264_configure(AVCodecContext *avctx) > priv->fixed_qp_p = 26; > priv->fixed_qp_b = 26; > > - av_log(avctx, AV_LOG_DEBUG, "Using %s-bitrate = %"PRId64" bps.\n", > - ctx->va_rc_mode == VA_RC_CBR ? "constant" : "variable", > - avctx->bit_rate); > - > } else { > av_assert0(0 && "Invalid RC mode."); > } > @@ -934,14 +930,6 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext > *avctx) > return AVERROR_PATCHWELCOME; > } > > - if (avctx->bit_rate > 0) { > - if (avctx->rc_max_rate == avctx->bit_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else > - ctx->va_rc_mode = VA_RC_CQP; > - > ctx->va_packed_headers = > VA_ENC_PACKED_HEADER_SEQUENCE | // SPS and PPS. > VA_ENC_PACKED_HEADER_SLICE | // Slice headers. > diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c > index b8b66b87cb..b296919b37 100644 > --- a/libavcodec/vaapi_encode_h265.c > +++ b/libavcodec/vaapi_encode_h265.c > @@ -512,7 +512,7 @@ static int > vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx) > .intra_period = avctx->gop_size, > .intra_idr_period = avctx->gop_size, > .ip_period = ctx->b_per_p + 1, > - .bits_per_second = avctx->bit_rate, > + .bits_per_second = ctx->va_bit_rate, > > .pic_width_in_luma_samples = sps->pic_width_in_luma_samples, > .pic_height_in_luma_samples = sps->pic_height_in_luma_samples, > @@ -1014,10 +1014,6 @@ static av_cold int > vaapi_encode_h265_configure(AVCodecContext *avctx) > priv->fixed_qp_p = 30; > priv->fixed_qp_b = 30; > > - av_log(avctx, AV_LOG_DEBUG, "Using %s-bitrate = %"PRId64" bps.\n", > - ctx->va_rc_mode == VA_RC_CBR ? "constant" : "variable", > - avctx->bit_rate); > - > } else { > av_assert0(0 && "Invalid RC mode."); > } > @@ -1068,14 +1064,6 @@ static av_cold int > vaapi_encode_h265_init(AVCodecContext *avctx) > if (avctx->level == FF_LEVEL_UNKNOWN) > avctx->level = priv->level; > > - if (avctx->bit_rate > 0) { > - if (avctx->rc_max_rate == avctx->bit_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else > - ctx->va_rc_mode = VA_RC_CQP; > - > ctx->va_packed_headers = > VA_ENC_PACKED_HEADER_SEQUENCE | // VPS, SPS and PPS. > VA_ENC_PACKED_HEADER_SLICE | // Slice headers. > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c > index b328beaa09..67ac2fba96 100644 > --- a/libavcodec/vaapi_encode_mjpeg.c > +++ b/libavcodec/vaapi_encode_mjpeg.c > @@ -388,8 +388,6 @@ static av_cold int vaapi_encode_mjpeg_init(AVCodecContext > *avctx) > > ctx->codec = &vaapi_encode_type_mjpeg; > > - ctx->va_rc_mode = VA_RC_CQP; > - > // The JPEG image header - see note above. > ctx->va_packed_headers = > VA_ENC_PACKED_HEADER_RAW_DATA; > @@ -402,6 +400,7 @@ static av_cold int vaapi_encode_mjpeg_init(AVCodecContext > *avctx) > > static const AVCodecDefault vaapi_encode_mjpeg_defaults[] = { > { "global_quality", "80" }, > + { "b", "0" }, > { NULL }, > }; > > diff --git a/libavcodec/vaapi_encode_mpeg2.c b/libavcodec/vaapi_encode_mpeg2.c > index db79d72115..ff86b8817e 100644 > --- a/libavcodec/vaapi_encode_mpeg2.c > +++ b/libavcodec/vaapi_encode_mpeg2.c > @@ -188,8 +188,8 @@ static int > vaapi_encode_mpeg2_init_sequence_params(AVCodecContext *avctx) > memset(pce, 0, sizeof(*pce)); > > > - if (avctx->bit_rate > 0) { > - priv->bit_rate = (avctx->bit_rate + 399) / 400; > + if (ctx->va_bit_rate > 0) { > + priv->bit_rate = (ctx->va_bit_rate + 399) / 400; > } else { > // Unknown (not a bitrate-targetting mode), so just use the > // highest value. > @@ -361,7 +361,7 @@ static int > vaapi_encode_mpeg2_init_sequence_params(AVCodecContext *avctx) > .picture_width = avctx->width, > .picture_height = avctx->height, > > - .bits_per_second = avctx->bit_rate, > + .bits_per_second = ctx->va_bit_rate, > .frame_rate = av_q2d(priv->frame_rate), > .aspect_ratio_information = sh->aspect_ratio_information, > .vbv_buffer_size = priv->vbv_buffer_size, > @@ -615,8 +615,6 @@ static av_cold int vaapi_encode_mpeg2_init(AVCodecContext > *avctx) > return AVERROR(EINVAL); > } > > - ctx->va_rc_mode = VA_RC_CQP; > - > ctx->va_packed_headers = VA_ENC_PACKED_HEADER_SEQUENCE | > VA_ENC_PACKED_HEADER_PICTURE; > > @@ -666,6 +664,7 @@ static const AVOption vaapi_encode_mpeg2_options[] = { > }; > > static const AVCodecDefault vaapi_encode_mpeg2_defaults[] = { > + { "b", "0" }, > { "bf", "1" }, > { "g", "120" }, > { "i_qfactor", "1" }, > diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c > index 9588826bfb..40871a6bbf 100644 > --- a/libavcodec/vaapi_encode_vp8.c > +++ b/libavcodec/vaapi_encode_vp8.c > @@ -65,7 +65,7 @@ static int > vaapi_encode_vp8_init_sequence_params(AVCodecContext *avctx) > vseq->kf_auto = 0; > > if (!(ctx->va_rc_mode & VA_RC_CQP)) { > - vseq->bits_per_second = avctx->bit_rate; > + vseq->bits_per_second = ctx->va_bit_rate; > vseq->intra_period = avctx->gop_size; > } > > @@ -205,17 +205,6 @@ static av_cold int vaapi_encode_vp8_init(AVCodecContext > *avctx) > > ctx->codec = &vaapi_encode_type_vp8; > > - if (avctx->flags & AV_CODEC_FLAG_QSCALE) { > - ctx->va_rc_mode = VA_RC_CQP; > - } else if (avctx->bit_rate > 0) { > - if (avctx->rc_max_rate == avctx->bit_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else { > - ctx->va_rc_mode = VA_RC_CQP; > - } > - > // Packed headers are not currently supported. > ctx->va_packed_headers = 0; > > diff --git a/libavcodec/vaapi_encode_vp9.c b/libavcodec/vaapi_encode_vp9.c > index 4d7cec0520..e400bc8b79 100644 > --- a/libavcodec/vaapi_encode_vp9.c > +++ b/libavcodec/vaapi_encode_vp9.c > @@ -71,7 +71,7 @@ static int > vaapi_encode_vp9_init_sequence_params(AVCodecContext *avctx) > vseq->kf_auto = 0; > > if (!(ctx->va_rc_mode & VA_RC_CQP)) { > - vseq->bits_per_second = avctx->bit_rate; > + vseq->bits_per_second = ctx->va_bit_rate; > vseq->intra_period = avctx->gop_size; > } > > @@ -227,17 +227,6 @@ static av_cold int vaapi_encode_vp9_init(AVCodecContext > *avctx) > > ctx->codec = &vaapi_encode_type_vp9; > > - if (avctx->flags & AV_CODEC_FLAG_QSCALE) { > - ctx->va_rc_mode = VA_RC_CQP; > - } else if (avctx->bit_rate > 0) { > - if (avctx->bit_rate == avctx->rc_max_rate) > - ctx->va_rc_mode = VA_RC_CBR; > - else > - ctx->va_rc_mode = VA_RC_VBR; > - } else { > - ctx->va_rc_mode = VA_RC_CQP; > - } > - > // Packed headers are not currently supported. > ctx->va_packed_headers = 0; The logic for rate control is more clear to me in this patch. Thanks Haihao
On 08/06/18 06:05, mypopy@gmail.com wrote: > On Fri, Jun 8, 2018 at 7:45 AM Mark Thompson <sw@jkqxz.net> wrote: >> >> Query which modes are supported and select between VBR and CBR based >> on that - this removes all of the codec-specific rate control mode >> selection code. >> --- >> doc/encoders.texi | 2 - >> libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++------------ >> libavcodec/vaapi_encode.h | 6 +- >> libavcodec/vaapi_encode_h264.c | 18 +---- >> libavcodec/vaapi_encode_h265.c | 14 +--- >> libavcodec/vaapi_encode_mjpeg.c | 3 +- >> libavcodec/vaapi_encode_mpeg2.c | 9 +-- >> libavcodec/vaapi_encode_vp8.c | 13 +-- >> libavcodec/vaapi_encode_vp9.c | 13 +-- >> 9 files changed, 137 insertions(+), 114 deletions(-) >> >> diff --git a/doc/encoders.texi b/doc/encoders.texi >> index 62a1509a96..0c0a307987 100644 >> ... >> @@ -1313,44 +1286,144 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) >> static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) >> { >> VAAPIEncodeContext *ctx = avctx->priv_data; >> - int rc_bits_per_second; >> - int rc_target_percentage; >> - int rc_window_size; >> - int hrd_buffer_size; >> - int hrd_initial_buffer_fullness; >> + int64_t rc_bits_per_second; >> + int rc_target_percentage; >> + int rc_window_size; >> + int64_t hrd_buffer_size; >> + int64_t hrd_initial_buffer_fullness; >> int fr_num, fr_den; >> + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; >> + VAStatus vas; >> + >> + vas = vaGetConfigAttributes(ctx->hwctx->display, >> + ctx->va_profile, ctx->va_entrypoint, >> + &rc_attr, 1); >> + if (vas != VA_STATUS_SUCCESS) { >> + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " >> + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); >> + return AVERROR_EXTERNAL; >> + } >> >> - if (avctx->bit_rate > INT32_MAX) { >> - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " >> - "higher is not supported.\n"); >> + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { >> + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " >> + "supported rate control modes: assuming constant-quality.\n"); >> + ctx->va_rc_mode = VA_RC_CQP; >> + return 0; >> + } >> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || >> + avctx->bit_rate <= 0) { >> + if (rc_attr.value & VA_RC_CQP) { >> + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality mode.\n"); >> + ctx->va_rc_mode = VA_RC_CQP; >> + return 0; >> + } else { >> + av_log(avctx, AV_LOG_ERROR, "Driver does not support " >> + "constant-quality mode (%#x).\n", rc_attr.value); >> + return AVERROR(EINVAL); >> + } >> + } >> + >> + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { >> + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " >> + "bitrate-targetted rate control modes.\n"); >> return AVERROR(EINVAL); >> } >> >> if (avctx->rc_buffer_size) >> hrd_buffer_size = avctx->rc_buffer_size; >> + else if (avctx->rc_max_rate > 0) >> + hrd_buffer_size = avctx->rc_max_rate; >> else >> hrd_buffer_size = avctx->bit_rate; >> - if (avctx->rc_initial_buffer_occupancy) >> + if (avctx->rc_initial_buffer_occupancy) { >> + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { >> + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " >> + "must have initial buffer size (%d) < " >> + "buffer size (%"PRId64").\n", >> + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); >> + return AVERROR(EINVAL); >> + } >> hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; >> - else >> + } else { >> hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; >> + } >> + >> + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { >> + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have " >> + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", >> + avctx->bit_rate, avctx->rc_max_rate); >> + return AVERROR(EINVAL); >> + } >> + >> + if (avctx->rc_max_rate > avctx->bit_rate) { >> + if (!(rc_attr.value & VA_RC_VBR)) { >> + av_log(avctx, AV_LOG_WARNING, "Driver does not support " >> + "VBR mode (%#x), using CBR mode instead.\n", >> + rc_attr.value); >> + ctx->va_rc_mode = VA_RC_CBR; >> + } else { >> + ctx->va_rc_mode = VA_RC_VBR; >> + } >> + >> + rc_bits_per_second = avctx->rc_max_rate; >> + rc_target_percentage = (avctx->bit_rate * 100) / avctx->rc_max_rate; >> + >> + } else if (avctx->rc_max_rate == avctx->bit_rate) { >> + if (!(rc_attr.value & VA_RC_CBR)) { >> + av_log(avctx, AV_LOG_WARNING, "Driver does not support " >> + "CBR mode (%#x), using VBR mode instead.\n", >> + rc_attr.value); >> + ctx->va_rc_mode = VA_RC_VBR; >> + } else { >> + ctx->va_rc_mode = VA_RC_CBR; >> + } >> >> - if (ctx->va_rc_mode == VA_RC_CBR) { >> rc_bits_per_second = avctx->bit_rate; >> rc_target_percentage = 100; >> - rc_window_size = 1000; >> + >> } else { >> - if (avctx->rc_max_rate < avctx->bit_rate) { >> - // Max rate is unset or invalid, just use the normal bitrate. >> + if (rc_attr.value & VA_RC_VBR) { >> + ctx->va_rc_mode = VA_RC_VBR; >> + >> + // We only have a target bitrate, but VAAPI requires that a >> + // maximum rate be supplied as well. Since the user has >> + // offered no particular constraint, arbitrarily pick a >> + // maximum rate of double the target rate. >> + rc_bits_per_second = 2 * avctx->bit_rate; >> + rc_target_percentage = 50; >> + } else { >> + ctx->va_rc_mode = VA_RC_CBR; >> + >> rc_bits_per_second = avctx->bit_rate; >> rc_target_percentage = 100; >> - } else { >> - rc_bits_per_second = avctx->rc_max_rate; >> - rc_target_percentage = (avctx->bit_rate * 100) / rc_bits_per_second; >> } >> - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; >> } >> >> + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; >> + >> + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " >> + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", >> + rc_target_percentage, rc_bits_per_second, rc_window_size); >> + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " >> + "initial fullness %"PRId64" bits.\n", >> + hrd_buffer_size, hrd_initial_buffer_fullness); >> + >> + if (rc_bits_per_second > UINT32_MAX || >> + hrd_buffer_size > UINT32_MAX || >> + hrd_initial_buffer_fullness > UINT32_MAX) { >> + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " >> + "greater are not supported by VAAPI.\n"); >> + return AVERROR(EINVAL); >> + } >> + > Use a assert0() maybe more pithily I don't think it can be - it's end-user-triggerable, and there isn't any way for an API user to detect that. (Consider "-b:v 1T".) Thanks, - Mark
On 13/06/18 08:03, Xiang, Haihao wrote: > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: >> Query which modes are supported and select between VBR and CBR based >> on that - this removes all of the codec-specific rate control mode >> selection code. >> --- >> doc/encoders.texi | 2 - >> libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++----------- >> - >> libavcodec/vaapi_encode.h | 6 +- >> libavcodec/vaapi_encode_h264.c | 18 +---- >> libavcodec/vaapi_encode_h265.c | 14 +--- >> libavcodec/vaapi_encode_mjpeg.c | 3 +- >> libavcodec/vaapi_encode_mpeg2.c | 9 +-- >> libavcodec/vaapi_encode_vp8.c | 13 +-- >> libavcodec/vaapi_encode_vp9.c | 13 +-- >> 9 files changed, 137 insertions(+), 114 deletions(-) >> >> ... >> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c >> index f4c063734c..5de5483454 100644 >> --- a/libavcodec/vaapi_encode.c >> +++ b/libavcodec/vaapi_encode.c >> ... >> @@ -1313,44 +1286,144 @@ static av_cold int >> vaapi_encode_config_attributes(AVCodecContext *avctx) >> static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) >> { >> VAAPIEncodeContext *ctx = avctx->priv_data; >> - int rc_bits_per_second; >> - int rc_target_percentage; >> - int rc_window_size; >> - int hrd_buffer_size; >> - int hrd_initial_buffer_fullness; >> + int64_t rc_bits_per_second; >> + int rc_target_percentage; >> + int rc_window_size; >> + int64_t hrd_buffer_size; >> + int64_t hrd_initial_buffer_fullness; >> int fr_num, fr_den; >> + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; >> + VAStatus vas; >> + >> + vas = vaGetConfigAttributes(ctx->hwctx->display, >> + ctx->va_profile, ctx->va_entrypoint, >> + &rc_attr, 1); >> + if (vas != VA_STATUS_SUCCESS) { >> + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " >> + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); >> + return AVERROR_EXTERNAL; >> + } >> >> - if (avctx->bit_rate > INT32_MAX) { >> - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " >> - "higher is not supported.\n"); >> + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { >> + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " >> + "supported rate control modes: assuming constant-quality.\n"); > > How about logging it as warning message? What would a user do about it? (Note that it gets logged for JPEG encoding on all versions of the i965 driver except the most recent, so if it were a warning it would be seen by everyone using those versions.) >> + ctx->va_rc_mode = VA_RC_CQP; >> + return 0; >> + } >> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || >> + avctx->bit_rate <= 0) { >> + if (rc_attr.value & VA_RC_CQP) { >> + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality mode.\n"); >> + ctx->va_rc_mode = VA_RC_CQP; >> + return 0; >> + } else { >> + av_log(avctx, AV_LOG_ERROR, "Driver does not support " >> + "constant-quality mode (%#x).\n", rc_attr.value); >> + return AVERROR(EINVAL); >> + } >> + } >> + >> + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { >> + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " >> + "bitrate-targetted rate control modes.\n"); >> return AVERROR(EINVAL); >> } >> >> if (avctx->rc_buffer_size) >> hrd_buffer_size = avctx->rc_buffer_size; >> + else if (avctx->rc_max_rate > 0) >> + hrd_buffer_size = avctx->rc_max_rate; >> else >> hrd_buffer_size = avctx->bit_rate; >> - if (avctx->rc_initial_buffer_occupancy) >> + if (avctx->rc_initial_buffer_occupancy) { >> + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { >> + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " >> + "must have initial buffer size (%d) < " >> + "buffer size (%"PRId64").\n", >> + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); >> + return AVERROR(EINVAL); >> + } >> hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; >> - else >> + } else { >> hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; >> + } >> + >> + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { >> + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have " >> + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", >> + avctx->bit_rate, avctx->rc_max_rate); >> + return AVERROR(EINVAL); >> + } >> + >> + if (avctx->rc_max_rate > avctx->bit_rate) { >> + if (!(rc_attr.value & VA_RC_VBR)) { >> + av_log(avctx, AV_LOG_WARNING, "Driver does not support " >> + "VBR mode (%#x), using CBR mode instead.\n", >> + rc_attr.value); >> + ctx->va_rc_mode = VA_RC_CBR; >> + } else { >> + ctx->va_rc_mode = VA_RC_VBR; >> + } >> + >> + rc_bits_per_second = avctx->rc_max_rate; >> + rc_target_percentage = (avctx->bit_rate * 100) / avctx->rc_max_rate; > > I think rc_target_percentage should be 100 for CBR case. Yes; fixed. The rc_bits_per_second value is also wrong in that case (shouldn't be rc_max_rate if we can't use it), fixed similarly. >> + >> + } else if (avctx->rc_max_rate == avctx->bit_rate) { >> + if (!(rc_attr.value & VA_RC_CBR)) { >> + av_log(avctx, AV_LOG_WARNING, "Driver does not support " >> + "CBR mode (%#x), using VBR mode instead.\n", >> + rc_attr.value); >> + ctx->va_rc_mode = VA_RC_VBR; >> + } else { >> + ctx->va_rc_mode = VA_RC_CBR; >> + } >> >> - if (ctx->va_rc_mode == VA_RC_CBR) { >> rc_bits_per_second = avctx->bit_rate; >> rc_target_percentage = 100; >> - rc_window_size = 1000; >> + >> } else { >> - if (avctx->rc_max_rate < avctx->bit_rate) { >> - // Max rate is unset or invalid, just use the normal bitrate. >> + if (rc_attr.value & VA_RC_VBR) { >> + ctx->va_rc_mode = VA_RC_VBR; > > Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR is supported > by driver? I don't think so? VBR with the specified target is probably what you want in most cases, and I think anyone with specific constraints that want constant bitrate should expect to set maxrate to achieve that. >> + >> + // We only have a target bitrate, but VAAPI requires that a >> + // maximum rate be supplied as well. Since the user has >> + // offered no particular constraint, arbitrarily pick a >> + // maximum rate of double the target rate. >> + rc_bits_per_second = 2 * avctx->bit_rate; >> + rc_target_percentage = 50; >> + } else { >> + ctx->va_rc_mode = VA_RC_CBR; >> + >> rc_bits_per_second = avctx->bit_rate; >> rc_target_percentage = 100; >> - } else { >> - rc_bits_per_second = avctx->rc_max_rate; >> - rc_target_percentage = (avctx->bit_rate * 100) / >> rc_bits_per_second; >> } >> - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; >> } >> >> + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; >> + >> + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " >> + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", >> + rc_target_percentage, rc_bits_per_second, rc_window_size); >> + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " >> + "initial fullness %"PRId64" bits.\n", >> + hrd_buffer_size, hrd_initial_buffer_fullness); >> + >> + if (rc_bits_per_second > UINT32_MAX || >> + hrd_buffer_size > UINT32_MAX || >> + hrd_initial_buffer_fullness > UINT32_MAX) { >> + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " >> + "greater are not supported by VAAPI.\n"); >> + return AVERROR(EINVAL); >> + } >> + >> + ctx->va_bit_rate = rc_bits_per_second; >> + >> + ctx->config_attributes[ctx->nb_config_attributes++] = >> + (VAConfigAttrib) { >> + .type = VAConfigAttribRateControl, >> + .value = ctx->va_rc_mode, >> + }; >> + >> ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl; >> ctx->rc_params.rc = (VAEncMiscParameterRateControl) { >> .bits_per_second = rc_bits_per_second, >> @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) >> if (err < 0) >> goto fail; >> >> + err = vaapi_encode_init_rate_control(avctx); >> + if (err < 0) >> + goto fail; >> + >> err = vaapi_encode_config_attributes(avctx); >> if (err < 0) >> goto fail; >> @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) >> goto fail; >> } >> >> - if (ctx->va_rc_mode & ~VA_RC_CQP) { >> - err = vaapi_encode_init_rate_control(avctx); >> - if (err < 0) >> - goto fail; >> - } >> - >> if (ctx->codec->configure) { >> err = ctx->codec->configure(avctx); >> if (err < 0) >> ... > > The logic for rate control is more clear to me in this patch. Thanks, - Mark
On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: > On 13/06/18 08:03, Xiang, Haihao wrote: > > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: > > > Query which modes are supported and select between VBR and CBR based > > > on that - this removes all of the codec-specific rate control mode > > > selection code. > > > --- > > > doc/encoders.texi | 2 - > > > libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++------- > > > ---- > > > - > > > libavcodec/vaapi_encode.h | 6 +- > > > libavcodec/vaapi_encode_h264.c | 18 +---- > > > libavcodec/vaapi_encode_h265.c | 14 +--- > > > libavcodec/vaapi_encode_mjpeg.c | 3 +- > > > libavcodec/vaapi_encode_mpeg2.c | 9 +-- > > > libavcodec/vaapi_encode_vp8.c | 13 +-- > > > libavcodec/vaapi_encode_vp9.c | 13 +-- > > > 9 files changed, 137 insertions(+), 114 deletions(-) > > > > > > ... > > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > > > index f4c063734c..5de5483454 100644 > > > --- a/libavcodec/vaapi_encode.c > > > +++ b/libavcodec/vaapi_encode.c > > > ... > > > @@ -1313,44 +1286,144 @@ static av_cold int > > > vaapi_encode_config_attributes(AVCodecContext *avctx) > > > static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) > > > { > > > VAAPIEncodeContext *ctx = avctx->priv_data; > > > - int rc_bits_per_second; > > > - int rc_target_percentage; > > > - int rc_window_size; > > > - int hrd_buffer_size; > > > - int hrd_initial_buffer_fullness; > > > + int64_t rc_bits_per_second; > > > + int rc_target_percentage; > > > + int rc_window_size; > > > + int64_t hrd_buffer_size; > > > + int64_t hrd_initial_buffer_fullness; > > > int fr_num, fr_den; > > > + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; > > > + VAStatus vas; > > > + > > > + vas = vaGetConfigAttributes(ctx->hwctx->display, > > > + ctx->va_profile, ctx->va_entrypoint, > > > + &rc_attr, 1); > > > + if (vas != VA_STATUS_SUCCESS) { > > > + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " > > > + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); > > > + return AVERROR_EXTERNAL; > > > + } > > > > > > - if (avctx->bit_rate > INT32_MAX) { > > > - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " > > > - "higher is not supported.\n"); > > > + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { > > > + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " > > > + "supported rate control modes: assuming constant- > > > quality.\n"); > > > > How about logging it as warning message? > > What would a user do about it? > User may know what is not supported in the driver and he/she might get wrong result, he/she may file an issue to the driver. > (Note that it gets logged for JPEG encoding on all versions of the i965 driver > except the most recent, so if it were a warning it would be seen by everyone > using those versions.) > > > > + ctx->va_rc_mode = VA_RC_CQP; > > > + return 0; > > > + } > > > + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > > > + avctx->bit_rate <= 0) { > > > + if (rc_attr.value & VA_RC_CQP) { > > > + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality > > > mode.\n"); > > > + ctx->va_rc_mode = VA_RC_CQP; > > > + return 0; > > > + } else { > > > + av_log(avctx, AV_LOG_ERROR, "Driver does not support " > > > + "constant-quality mode (%#x).\n", rc_attr.value); > > > + return AVERROR(EINVAL); > > > + } > > > + } > > > + > > > + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { > > > + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " > > > + "bitrate-targetted rate control modes.\n"); > > > return AVERROR(EINVAL); > > > } > > > > > > if (avctx->rc_buffer_size) > > > hrd_buffer_size = avctx->rc_buffer_size; > > > + else if (avctx->rc_max_rate > 0) > > > + hrd_buffer_size = avctx->rc_max_rate; > > > else > > > hrd_buffer_size = avctx->bit_rate; > > > - if (avctx->rc_initial_buffer_occupancy) > > > + if (avctx->rc_initial_buffer_occupancy) { > > > + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { > > > + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " > > > + "must have initial buffer size (%d) < " > > > + "buffer size (%"PRId64").\n", > > > + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); > > > + return AVERROR(EINVAL); > > > + } > > > hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; > > > - else > > > + } else { > > > hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; > > > + } > > > + > > > + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { > > > + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have > > > " > > > + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", > > > + avctx->bit_rate, avctx->rc_max_rate); > > > + return AVERROR(EINVAL); > > > + } > > > + > > > + if (avctx->rc_max_rate > avctx->bit_rate) { > > > + if (!(rc_attr.value & VA_RC_VBR)) { > > > + av_log(avctx, AV_LOG_WARNING, "Driver does not support " > > > + "VBR mode (%#x), using CBR mode instead.\n", > > > + rc_attr.value); > > > + ctx->va_rc_mode = VA_RC_CBR; > > > + } else { > > > + ctx->va_rc_mode = VA_RC_VBR; > > > + } > > > + > > > + rc_bits_per_second = avctx->rc_max_rate; > > > + rc_target_percentage = (avctx->bit_rate * 100) / avctx- > > > >rc_max_rate; > > > > I think rc_target_percentage should be 100 for CBR case. > > Yes; fixed. > > The rc_bits_per_second value is also wrong in that case (shouldn't be > rc_max_rate if we can't use it), fixed similarly. > > > > + > > > + } else if (avctx->rc_max_rate == avctx->bit_rate) { > > > + if (!(rc_attr.value & VA_RC_CBR)) { > > > + av_log(avctx, AV_LOG_WARNING, "Driver does not support " > > > + "CBR mode (%#x), using VBR mode instead.\n", > > > + rc_attr.value); > > > + ctx->va_rc_mode = VA_RC_VBR; > > > + } else { > > > + ctx->va_rc_mode = VA_RC_CBR; > > > + } > > > > > > - if (ctx->va_rc_mode == VA_RC_CBR) { > > > rc_bits_per_second = avctx->bit_rate; > > > rc_target_percentage = 100; > > > - rc_window_size = 1000; > > > + > > > } else { > > > - if (avctx->rc_max_rate < avctx->bit_rate) { > > > - // Max rate is unset or invalid, just use the normal bitrate. > > > + if (rc_attr.value & VA_RC_VBR) { > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR is > > supported > > by driver? > > I don't think so? VBR with the specified target is probably what you want in > most cases, and I think anyone with specific constraints that want constant > bitrate should expect to set maxrate to achieve that. > I agree VBR is probably what an user wants in most case, however target percent set to 50% is not suitable for most case. To get a specific target percent, user should set both target bitrate and max bitrate, so it is reasonable to ask user must set both target bitrate and max bitrate for VBR cases, and for CBR user may set target bitrate only. I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to keep consistency with QSV for this case. Thanks Haihao > > > + > > > + // We only have a target bitrate, but VAAPI requires that a > > > + // maximum rate be supplied as well. Since the user has > > > + // offered no particular constraint, arbitrarily pick a > > > + // maximum rate of double the target rate. > > > + rc_bits_per_second = 2 * avctx->bit_rate; > > > + rc_target_percentage = 50; > > > + } else { > > > + ctx->va_rc_mode = VA_RC_CBR; > > > + > > > rc_bits_per_second = avctx->bit_rate; > > > rc_target_percentage = 100; > > > - } else { > > > - rc_bits_per_second = avctx->rc_max_rate; > > > - rc_target_percentage = (avctx->bit_rate * 100) / > > > rc_bits_per_second; > > > } > > > - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; > > > } > > > > > > + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; > > > + > > > + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " > > > + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", > > > + rc_target_percentage, rc_bits_per_second, rc_window_size); > > > + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " > > > + "initial fullness %"PRId64" bits.\n", > > > + hrd_buffer_size, hrd_initial_buffer_fullness); > > > + > > > + if (rc_bits_per_second > UINT32_MAX || > > > + hrd_buffer_size > UINT32_MAX || > > > + hrd_initial_buffer_fullness > UINT32_MAX) { > > > + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " > > > + "greater are not supported by VAAPI.\n"); > > > + return AVERROR(EINVAL); > > > + } > > > + > > > + ctx->va_bit_rate = rc_bits_per_second; > > > + > > > + ctx->config_attributes[ctx->nb_config_attributes++] = > > > + (VAConfigAttrib) { > > > + .type = VAConfigAttribRateControl, > > > + .value = ctx->va_rc_mode, > > > + }; > > > + > > > ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl; > > > ctx->rc_params.rc = (VAEncMiscParameterRateControl) { > > > .bits_per_second = rc_bits_per_second, > > > @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext > > > *avctx) > > > if (err < 0) > > > goto fail; > > > > > > + err = vaapi_encode_init_rate_control(avctx); > > > + if (err < 0) > > > + goto fail; > > > + > > > err = vaapi_encode_config_attributes(avctx); > > > if (err < 0) > > > goto fail; > > > @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext > > > *avctx) > > > goto fail; > > > } > > > > > > - if (ctx->va_rc_mode & ~VA_RC_CQP) { > > > - err = vaapi_encode_init_rate_control(avctx); > > > - if (err < 0) > > > - goto fail; > > > - } > > > - > > > if (ctx->codec->configure) { > > > err = ctx->codec->configure(avctx); > > > if (err < 0) > > > ... > > > > The logic for rate control is more clear to me in this patch. > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Xiang, Haihao > Sent: Thursday, June 14, 2018 2:08 PM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate > control configuration > > On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: > > On 13/06/18 08:03, Xiang, Haihao wrote: > > > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: > > > > Query which modes are supported and select between VBR and CBR > > > > based on that - this removes all of the codec-specific rate > > > > control mode selection code. > > > > --- > > > > doc/encoders.texi | 2 - > > > > libavcodec/vaapi_encode.c | 173 > ++++++++++++++++++++++++++++------- > > > > ---- > > > > - > > > > libavcodec/vaapi_encode.h | 6 +- > > > > libavcodec/vaapi_encode_h264.c | 18 +---- > > > > libavcodec/vaapi_encode_h265.c | 14 +--- > > > > libavcodec/vaapi_encode_mjpeg.c | 3 +- > > > > libavcodec/vaapi_encode_mpeg2.c | 9 +-- > > > > libavcodec/vaapi_encode_vp8.c | 13 +-- > > > > libavcodec/vaapi_encode_vp9.c | 13 +-- > > > > 9 files changed, 137 insertions(+), 114 deletions(-) > > > > > > > > ... > > > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > > > > index f4c063734c..5de5483454 100644 > > > > --- a/libavcodec/vaapi_encode.c > > > > +++ b/libavcodec/vaapi_encode.c > > > > ... > > > > @@ -1313,44 +1286,144 @@ static av_cold int > > > > vaapi_encode_config_attributes(AVCodecContext *avctx) static > > > > av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) > > > > { > > > > VAAPIEncodeContext *ctx = avctx->priv_data; > > > > - int rc_bits_per_second; > > > > - int rc_target_percentage; > > > > - int rc_window_size; > > > > - int hrd_buffer_size; > > > > - int hrd_initial_buffer_fullness; > > > > + int64_t rc_bits_per_second; > > > > + int rc_target_percentage; > > > > + int rc_window_size; > > > > + int64_t hrd_buffer_size; > > > > + int64_t hrd_initial_buffer_fullness; > > > > int fr_num, fr_den; > > > > + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; > > > > + VAStatus vas; > > > > + > > > > + vas = vaGetConfigAttributes(ctx->hwctx->display, > > > > + ctx->va_profile, > ctx->va_entrypoint, > > > > + &rc_attr, 1); > > > > + if (vas != VA_STATUS_SUCCESS) { > > > > + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control > " > > > > + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); > > > > + return AVERROR_EXTERNAL; > > > > + } > > > > > > > > - if (avctx->bit_rate > INT32_MAX) { > > > > - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps > or " > > > > - "higher is not supported.\n"); > > > > + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { > > > > + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report > any " > > > > + "supported rate control modes: assuming constant- > > > > quality.\n"); > > > > > > How about logging it as warning message? > > > > What would a user do about it? > > > > User may know what is not supported in the driver and he/she might get > wrong result, he/she may file an issue to the driver. > > > (Note that it gets logged for JPEG encoding on all versions of the > > i965 driver except the most recent, so if it were a warning it would > > be seen by everyone using those versions.) > > > > > > + ctx->va_rc_mode = VA_RC_CQP; > > > > + return 0; > > > > + } > > > > + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > > > > + avctx->bit_rate <= 0) { > > > > + if (rc_attr.value & VA_RC_CQP) { > > > > + av_log(avctx, AV_LOG_VERBOSE, "Using > constant-quality > > > > mode.\n"); > > > > + ctx->va_rc_mode = VA_RC_CQP; > > > > + return 0; > > > > + } else { > > > > + av_log(avctx, AV_LOG_ERROR, "Driver does not > support " > > > > + "constant-quality mode (%#x).\n", > rc_attr.value); > > > > + return AVERROR(EINVAL); > > > > + } > > > > + } > > > > + > > > > + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { > > > > + av_log(avctx, AV_LOG_ERROR, "Driver does not support any > " > > > > + "bitrate-targetted rate control modes.\n"); > > > > return AVERROR(EINVAL); > > > > } > > > > > > > > if (avctx->rc_buffer_size) > > > > hrd_buffer_size = avctx->rc_buffer_size; > > > > + else if (avctx->rc_max_rate > 0) > > > > + hrd_buffer_size = avctx->rc_max_rate; > > > > else > > > > hrd_buffer_size = avctx->bit_rate; > > > > - if (avctx->rc_initial_buffer_occupancy) > > > > + if (avctx->rc_initial_buffer_occupancy) { > > > > + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { > > > > + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer > settings: " > > > > + "must have initial buffer size (%d) < " > > > > + "buffer size (%"PRId64").\n", > > > > + avctx->rc_initial_buffer_occupancy, > hrd_buffer_size); > > > > + return AVERROR(EINVAL); > > > > + } > > > > hrd_initial_buffer_fullness = > avctx->rc_initial_buffer_occupancy; > > > > - else > > > > + } else { > > > > hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; > > > > + } > > > > + > > > > + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) > { > > > > + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: > > > > + must have > > > > " > > > > + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", > > > > + avctx->bit_rate, avctx->rc_max_rate); > > > > + return AVERROR(EINVAL); > > > > + } > > > > + > > > > + if (avctx->rc_max_rate > avctx->bit_rate) { > > > > + if (!(rc_attr.value & VA_RC_VBR)) { > > > > + av_log(avctx, AV_LOG_WARNING, "Driver does not > support " > > > > + "VBR mode (%#x), using CBR mode > instead.\n", > > > > + rc_attr.value); > > > > + ctx->va_rc_mode = VA_RC_CBR; > > > > + } else { > > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > + } > > > > + > > > > + rc_bits_per_second = avctx->rc_max_rate; > > > > + rc_target_percentage = (avctx->bit_rate * 100) / avctx- > > > > >rc_max_rate; > > > > > > I think rc_target_percentage should be 100 for CBR case. > > > > Yes; fixed. > > > > The rc_bits_per_second value is also wrong in that case (shouldn't be > > rc_max_rate if we can't use it), fixed similarly. > > > > > > + > > > > + } else if (avctx->rc_max_rate == avctx->bit_rate) { > > > > + if (!(rc_attr.value & VA_RC_CBR)) { > > > > + av_log(avctx, AV_LOG_WARNING, "Driver does not > support " > > > > + "CBR mode (%#x), using VBR mode > instead.\n", > > > > + rc_attr.value); > > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > + } else { > > > > + ctx->va_rc_mode = VA_RC_CBR; > > > > + } > > > > > > > > - if (ctx->va_rc_mode == VA_RC_CBR) { > > > > rc_bits_per_second = avctx->bit_rate; > > > > rc_target_percentage = 100; > > > > - rc_window_size = 1000; > > > > + > > > > } else { > > > > - if (avctx->rc_max_rate < avctx->bit_rate) { > > > > - // Max rate is unset or invalid, just use the normal > bitrate. > > > > + if (rc_attr.value & VA_RC_VBR) { > > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > > > Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR > > > is supported by driver? > > > > I don't think so? VBR with the specified target is probably what you > > want in most cases, and I think anyone with specific constraints that > > want constant bitrate should expect to set maxrate to achieve that. > > > > I agree VBR is probably what an user wants in most case, however target > percent set to 50% is not suitable for most case. To get a specific target > percent, user should set both target bitrate and max bitrate, so it is > reasonable to ask user must set both target bitrate and max bitrate for VBR > cases, and for CBR user may set target bitrate only. How about set the max_rate to be a very larger number such as INT_MAX if user hasn't set it? User may don't set max_rate on purpose, expecting better quality with unlimited bitrate fluctuation (common requirement for local video files). Double of target_bit_rate is too strict IMHO. And I haven't such a limitation in x264 ABR mode. > > I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to keep > consistency with QSV for this case. What will happen if user set a max_rate but without a setting for target_bitrate? The mode will be VBR (if driver support) with target_bitrate=0. Tried this on qsv, MSDK returns an error of invalid video parameters. Is it ok for vaapi? And also with iHD driver? > > Thanks > Haihao > > > > > > + > > > > + // We only have a target bitrate, but VAAPI requires > that a > > > > + // maximum rate be supplied as well. Since the user > has > > > > + // offered no particular constraint, arbitrarily pick a > > > > + // maximum rate of double the target rate. > > > > + rc_bits_per_second = 2 * avctx->bit_rate; > > > > + rc_target_percentage = 50; > > > > + } else { > > > > + ctx->va_rc_mode = VA_RC_CBR; > > > > + > > > > rc_bits_per_second = avctx->bit_rate; > > > > rc_target_percentage = 100; > > > > - } else { > > > > - rc_bits_per_second = avctx->rc_max_rate; > > > > - rc_target_percentage = (avctx->bit_rate * 100) / > > > > rc_bits_per_second; > > > > } > > > > - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; > > > > } > > > > > > > > + rc_window_size = (hrd_buffer_size * 1000) / > > > > + rc_bits_per_second; > > > > + > > > > + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% > of %"PRId64" bps " > > > > + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? > "VBR" : "CBR", > > > > + rc_target_percentage, rc_bits_per_second, > rc_window_size); > > > > + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " > > > > + "initial fullness %"PRId64" bits.\n", > > > > + hrd_buffer_size, hrd_initial_buffer_fullness); > > > > + > > > > + if (rc_bits_per_second > UINT32_MAX || > > > > + hrd_buffer_size > UINT32_MAX || > > > > + hrd_initial_buffer_fullness > UINT32_MAX) { > > > > + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " > > > > + "greater are not supported by VAAPI.\n"); > > > > + return AVERROR(EINVAL); > > > > + } > > > > + > > > > + ctx->va_bit_rate = rc_bits_per_second; > > > > + > > > > + ctx->config_attributes[ctx->nb_config_attributes++] = > > > > + (VAConfigAttrib) { > > > > + .type = VAConfigAttribRateControl, > > > > + .value = ctx->va_rc_mode, > > > > + }; > > > > + > > > > ctx->rc_params.misc.type = > VAEncMiscParameterTypeRateControl; > > > > ctx->rc_params.rc = (VAEncMiscParameterRateControl) { > > > > .bits_per_second = rc_bits_per_second, > > > > @@ -1611,6 +1684,10 @@ av_cold int > > > > ff_vaapi_encode_init(AVCodecContext > > > > *avctx) > > > > if (err < 0) > > > > goto fail; > > > > > > > > + err = vaapi_encode_init_rate_control(avctx); > > > > + if (err < 0) > > > > + goto fail; > > > > + > > > > err = vaapi_encode_config_attributes(avctx); > > > > if (err < 0) > > > > goto fail; > > > > @@ -1658,12 +1735,6 @@ av_cold int > > > > ff_vaapi_encode_init(AVCodecContext > > > > *avctx) > > > > goto fail; > > > > } > > > > > > > > - if (ctx->va_rc_mode & ~VA_RC_CQP) { > > > > - err = vaapi_encode_init_rate_control(avctx); > > > > - if (err < 0) > > > > - goto fail; > > > > - } > > > > - > > > > if (ctx->codec->configure) { > > > > err = ctx->codec->configure(avctx); > > > > if (err < 0) > > > > ... > > > > > > The logic for rate control is more clear to me in this patch. > > > > Thanks, > > > > - Mark
On Thu, 2018-06-14 at 07:22 +0000, Li, Zhong wrote: > > -----Original Message----- > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > > Of Xiang, Haihao > > Sent: Thursday, June 14, 2018 2:08 PM > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate > > control configuration > > > > On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: > > > On 13/06/18 08:03, Xiang, Haihao wrote: > > > > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: > > > > > Query which modes are supported and select between VBR and CBR > > > > > based on that - this removes all of the codec-specific rate > > > > > control mode selection code. > > > > > --- > > > > > doc/encoders.texi | 2 - > > > > > libavcodec/vaapi_encode.c | 173 > > > > ++++++++++++++++++++++++++++------- > > > > > ---- > > > > > - > > > > > libavcodec/vaapi_encode.h | 6 +- > > > > > libavcodec/vaapi_encode_h264.c | 18 +---- > > > > > libavcodec/vaapi_encode_h265.c | 14 +--- > > > > > libavcodec/vaapi_encode_mjpeg.c | 3 +- > > > > > libavcodec/vaapi_encode_mpeg2.c | 9 +-- > > > > > libavcodec/vaapi_encode_vp8.c | 13 +-- > > > > > libavcodec/vaapi_encode_vp9.c | 13 +-- > > > > > 9 files changed, 137 insertions(+), 114 deletions(-) > > > > > > > > > > ... > > > > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > > > > > index f4c063734c..5de5483454 100644 > > > > > --- a/libavcodec/vaapi_encode.c > > > > > +++ b/libavcodec/vaapi_encode.c > > > > > ... > > > > > @@ -1313,44 +1286,144 @@ static av_cold int > > > > > vaapi_encode_config_attributes(AVCodecContext *avctx) static > > > > > av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) > > > > > { > > > > > VAAPIEncodeContext *ctx = avctx->priv_data; > > > > > - int rc_bits_per_second; > > > > > - int rc_target_percentage; > > > > > - int rc_window_size; > > > > > - int hrd_buffer_size; > > > > > - int hrd_initial_buffer_fullness; > > > > > + int64_t rc_bits_per_second; > > > > > + int rc_target_percentage; > > > > > + int rc_window_size; > > > > > + int64_t hrd_buffer_size; > > > > > + int64_t hrd_initial_buffer_fullness; > > > > > int fr_num, fr_den; > > > > > + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; > > > > > + VAStatus vas; > > > > > + > > > > > + vas = vaGetConfigAttributes(ctx->hwctx->display, > > > > > + ctx->va_profile, > > > > ctx->va_entrypoint, > > > > > + &rc_attr, 1); > > > > > + if (vas != VA_STATUS_SUCCESS) { > > > > > + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control > > > > " > > > > > + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); > > > > > + return AVERROR_EXTERNAL; > > > > > + } > > > > > > > > > > - if (avctx->bit_rate > INT32_MAX) { > > > > > - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps > > > > or " > > > > > - "higher is not supported.\n"); > > > > > + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { > > > > > + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report > > > > any " > > > > > + "supported rate control modes: assuming constant- > > > > > quality.\n"); > > > > > > > > How about logging it as warning message? > > > > > > What would a user do about it? > > > > > > > User may know what is not supported in the driver and he/she might get > > wrong result, he/she may file an issue to the driver. > > > > > (Note that it gets logged for JPEG encoding on all versions of the > > > i965 driver except the most recent, so if it were a warning it would > > > be seen by everyone using those versions.) > > > > > > > > + ctx->va_rc_mode = VA_RC_CQP; > > > > > + return 0; > > > > > + } > > > > > + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > > > > > + avctx->bit_rate <= 0) { > > > > > + if (rc_attr.value & VA_RC_CQP) { > > > > > + av_log(avctx, AV_LOG_VERBOSE, "Using > > > > constant-quality > > > > > mode.\n"); > > > > > + ctx->va_rc_mode = VA_RC_CQP; > > > > > + return 0; > > > > > + } else { > > > > > + av_log(avctx, AV_LOG_ERROR, "Driver does not > > > > support " > > > > > + "constant-quality mode (%#x).\n", > > > > rc_attr.value); > > > > > + return AVERROR(EINVAL); > > > > > + } > > > > > + } > > > > > + > > > > > + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { > > > > > + av_log(avctx, AV_LOG_ERROR, "Driver does not support any > > > > " > > > > > + "bitrate-targetted rate control modes.\n"); > > > > > return AVERROR(EINVAL); > > > > > } > > > > > > > > > > if (avctx->rc_buffer_size) > > > > > hrd_buffer_size = avctx->rc_buffer_size; > > > > > + else if (avctx->rc_max_rate > 0) > > > > > + hrd_buffer_size = avctx->rc_max_rate; > > > > > else > > > > > hrd_buffer_size = avctx->bit_rate; > > > > > - if (avctx->rc_initial_buffer_occupancy) > > > > > + if (avctx->rc_initial_buffer_occupancy) { > > > > > + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { > > > > > + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer > > > > settings: " > > > > > + "must have initial buffer size (%d) < " > > > > > + "buffer size (%"PRId64").\n", > > > > > + avctx->rc_initial_buffer_occupancy, > > > > hrd_buffer_size); > > > > > + return AVERROR(EINVAL); > > > > > + } > > > > > hrd_initial_buffer_fullness = > > > > avctx->rc_initial_buffer_occupancy; > > > > > - else > > > > > + } else { > > > > > hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; > > > > > + } > > > > > + > > > > > + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) > > > > { > > > > > + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: > > > > > + must have > > > > > " > > > > > + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", > > > > > + avctx->bit_rate, avctx->rc_max_rate); > > > > > + return AVERROR(EINVAL); > > > > > + } > > > > > + > > > > > + if (avctx->rc_max_rate > avctx->bit_rate) { > > > > > + if (!(rc_attr.value & VA_RC_VBR)) { > > > > > + av_log(avctx, AV_LOG_WARNING, "Driver does not > > > > support " > > > > > + "VBR mode (%#x), using CBR mode > > > > instead.\n", > > > > > + rc_attr.value); > > > > > + ctx->va_rc_mode = VA_RC_CBR; > > > > > + } else { > > > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > > + } > > > > > + > > > > > + rc_bits_per_second = avctx->rc_max_rate; > > > > > + rc_target_percentage = (avctx->bit_rate * 100) / avctx- > > > > > > rc_max_rate; > > > > > > > > I think rc_target_percentage should be 100 for CBR case. > > > > > > Yes; fixed. > > > > > > The rc_bits_per_second value is also wrong in that case (shouldn't be > > > rc_max_rate if we can't use it), fixed similarly. > > > > > > > > + > > > > > + } else if (avctx->rc_max_rate == avctx->bit_rate) { > > > > > + if (!(rc_attr.value & VA_RC_CBR)) { > > > > > + av_log(avctx, AV_LOG_WARNING, "Driver does not > > > > support " > > > > > + "CBR mode (%#x), using VBR mode > > > > instead.\n", > > > > > + rc_attr.value); > > > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > > + } else { > > > > > + ctx->va_rc_mode = VA_RC_CBR; > > > > > + } > > > > > > > > > > - if (ctx->va_rc_mode == VA_RC_CBR) { > > > > > rc_bits_per_second = avctx->bit_rate; > > > > > rc_target_percentage = 100; > > > > > - rc_window_size = 1000; > > > > > + > > > > > } else { > > > > > - if (avctx->rc_max_rate < avctx->bit_rate) { > > > > > - // Max rate is unset or invalid, just use the normal > > > > bitrate. > > > > > + if (rc_attr.value & VA_RC_VBR) { > > > > > + ctx->va_rc_mode = VA_RC_VBR; > > > > > > > > Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR > > > > is supported by driver? > > > > > > I don't think so? VBR with the specified target is probably what you > > > want in most cases, and I think anyone with specific constraints that > > > want constant bitrate should expect to set maxrate to achieve that. > > > > > > > I agree VBR is probably what an user wants in most case, however target > > percent set to 50% is not suitable for most case. To get a specific target > > percent, user should set both target bitrate and max bitrate, so it is > > reasonable to ask user must set both target bitrate and max bitrate for VBR > > cases, and for CBR user may set target bitrate only. > > How about set the max_rate to be a very larger number such as INT_MAX if user > hasn't set it? > User may don't set max_rate on purpose, expecting better quality with > unlimited bitrate fluctuation (common requirement for local video files). > Double of target_bit_rate is too strict IMHO. And I haven't such a limitation > in x264 ABR mode. > The max bitrate is used to limit the peak bitrate, so setting the max bitrate to INT_MAX doesn't make sense to me. > > > > I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to > > keep > > consistency with QSV for this case. > > What will happen if user set a max_rate but without a setting for > target_bitrate? > The mode will be VBR (if driver support) with target_bitrate=0. > Tried this on qsv, MSDK returns an error of invalid video parameters. > Is it ok for vaapi? And also with iHD driver? > It is taken as CQP in ffmpeg VAAPI when target bitrate is 0 after applying this patch, so it should work with iHD driver. > > > > Thanks > > Haihao > > > > > > > > > + > > > > > + // We only have a target bitrate, but VAAPI requires > > > > that a > > > > > + // maximum rate be supplied as well. Since the user > > > > has > > > > > + // offered no particular constraint, arbitrarily pick a > > > > > + // maximum rate of double the target rate. > > > > > + rc_bits_per_second = 2 * avctx->bit_rate; > > > > > + rc_target_percentage = 50; > > > > > + } else { > > > > > + ctx->va_rc_mode = VA_RC_CBR; > > > > > + > > > > > rc_bits_per_second = avctx->bit_rate; > > > > > rc_target_percentage = 100; > > > > > - } else { > > > > > - rc_bits_per_second = avctx->rc_max_rate; > > > > > - rc_target_percentage = (avctx->bit_rate * 100) / > > > > > rc_bits_per_second; > > > > > } > > > > > - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; > > > > > } > > > > > > > > > > + rc_window_size = (hrd_buffer_size * 1000) / > > > > > + rc_bits_per_second; > > > > > + > > > > > + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% > > > > of %"PRId64" bps " > > > > > + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? > > > > "VBR" : "CBR", > > > > > + rc_target_percentage, rc_bits_per_second, > > > > rc_window_size); > > > > > + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " > > > > > + "initial fullness %"PRId64" bits.\n", > > > > > + hrd_buffer_size, hrd_initial_buffer_fullness); > > > > > + > > > > > + if (rc_bits_per_second > UINT32_MAX || > > > > > + hrd_buffer_size > UINT32_MAX || > > > > > + hrd_initial_buffer_fullness > UINT32_MAX) { > > > > > + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " > > > > > + "greater are not supported by VAAPI.\n"); > > > > > + return AVERROR(EINVAL); > > > > > + } > > > > > + > > > > > + ctx->va_bit_rate = rc_bits_per_second; > > > > > + > > > > > + ctx->config_attributes[ctx->nb_config_attributes++] = > > > > > + (VAConfigAttrib) { > > > > > + .type = VAConfigAttribRateControl, > > > > > + .value = ctx->va_rc_mode, > > > > > + }; > > > > > + > > > > > ctx->rc_params.misc.type = > > > > VAEncMiscParameterTypeRateControl; > > > > > ctx->rc_params.rc = (VAEncMiscParameterRateControl) { > > > > > .bits_per_second = rc_bits_per_second, > > > > > @@ -1611,6 +1684,10 @@ av_cold int > > > > > ff_vaapi_encode_init(AVCodecContext > > > > > *avctx) > > > > > if (err < 0) > > > > > goto fail; > > > > > > > > > > + err = vaapi_encode_init_rate_control(avctx); > > > > > + if (err < 0) > > > > > + goto fail; > > > > > + > > > > > err = vaapi_encode_config_attributes(avctx); > > > > > if (err < 0) > > > > > goto fail; > > > > > @@ -1658,12 +1735,6 @@ av_cold int > > > > > ff_vaapi_encode_init(AVCodecContext > > > > > *avctx) > > > > > goto fail; > > > > > } > > > > > > > > > > - if (ctx->va_rc_mode & ~VA_RC_CQP) { > > > > > - err = vaapi_encode_init_rate_control(avctx); > > > > > - if (err < 0) > > > > > - goto fail; > > > > > - } > > > > > - > > > > > if (ctx->codec->configure) { > > > > > err = ctx->codec->configure(avctx); > > > > > if (err < 0) > > > > > ... > > > > > > > > The logic for rate control is more clear to me in this patch. > > > > > > Thanks, > > > > > > - Mark > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 14/06/18 07:07, Xiang, Haihao wrote: > On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: >> On 13/06/18 08:03, Xiang, Haihao wrote: >>> On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: >>>> Query which modes are supported and select between VBR and CBR based >>>> on that - this removes all of the codec-specific rate control mode >>>> selection code. >>>> --- >>>> doc/encoders.texi | 2 - >>>> libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++------- >>>> ---- >>>> - >>>> libavcodec/vaapi_encode.h | 6 +- >>>> libavcodec/vaapi_encode_h264.c | 18 +---- >>>> libavcodec/vaapi_encode_h265.c | 14 +--- >>>> libavcodec/vaapi_encode_mjpeg.c | 3 +- >>>> libavcodec/vaapi_encode_mpeg2.c | 9 +-- >>>> libavcodec/vaapi_encode_vp8.c | 13 +-- >>>> libavcodec/vaapi_encode_vp9.c | 13 +-- >>>> 9 files changed, 137 insertions(+), 114 deletions(-) >>>> >>>> ... >>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c >>>> index f4c063734c..5de5483454 100644 >>>> --- a/libavcodec/vaapi_encode.c >>>> +++ b/libavcodec/vaapi_encode.c >>>> ... >>>> @@ -1313,44 +1286,144 @@ static av_cold int >>>> vaapi_encode_config_attributes(AVCodecContext *avctx) >>>> static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) >>>> { >>>> VAAPIEncodeContext *ctx = avctx->priv_data; >>>> - int rc_bits_per_second; >>>> - int rc_target_percentage; >>>> - int rc_window_size; >>>> - int hrd_buffer_size; >>>> - int hrd_initial_buffer_fullness; >>>> + int64_t rc_bits_per_second; >>>> + int rc_target_percentage; >>>> + int rc_window_size; >>>> + int64_t hrd_buffer_size; >>>> + int64_t hrd_initial_buffer_fullness; >>>> int fr_num, fr_den; >>>> + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; >>>> + VAStatus vas; >>>> + >>>> + vas = vaGetConfigAttributes(ctx->hwctx->display, >>>> + ctx->va_profile, ctx->va_entrypoint, >>>> + &rc_attr, 1); >>>> + if (vas != VA_STATUS_SUCCESS) { >>>> + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " >>>> + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); >>>> + return AVERROR_EXTERNAL; >>>> + } >>>> >>>> - if (avctx->bit_rate > INT32_MAX) { >>>> - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " >>>> - "higher is not supported.\n"); >>>> + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { >>>> + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " >>>> + "supported rate control modes: assuming constant- >>>> quality.\n"); >>> >>> How about logging it as warning message? >> >> What would a user do about it? >> > > User may know what is not supported in the driver and he/she might get wrong > result, he/she may file an issue to the driver. If it's broken then the verbose logging provides the answer. If it isn't broken, as in >> (Note that it gets logged for JPEG encoding on all versions of the i965 driver >> except the most recent, so if it were a warning it would be seen by everyone >> using those versions.) then the warning is unhelpful. >>>> + ctx->va_rc_mode = VA_RC_CQP; >>>> + return 0; >>>> + } >>>> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || >>>> + avctx->bit_rate <= 0) { >>>> + if (rc_attr.value & VA_RC_CQP) { >>>> + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality >>>> mode.\n"); >>>> + ctx->va_rc_mode = VA_RC_CQP; >>>> + return 0; >>>> + } else { >>>> + av_log(avctx, AV_LOG_ERROR, "Driver does not support " >>>> + "constant-quality mode (%#x).\n", rc_attr.value); >>>> + return AVERROR(EINVAL); >>>> + } >>>> + } >>>> + >>>> + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { >>>> + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " >>>> + "bitrate-targetted rate control modes.\n"); >>>> return AVERROR(EINVAL); >>>> } >>>> >>>> if (avctx->rc_buffer_size) >>>> hrd_buffer_size = avctx->rc_buffer_size; >>>> + else if (avctx->rc_max_rate > 0) >>>> + hrd_buffer_size = avctx->rc_max_rate; >>>> else >>>> hrd_buffer_size = avctx->bit_rate; >>>> - if (avctx->rc_initial_buffer_occupancy) >>>> + if (avctx->rc_initial_buffer_occupancy) { >>>> + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { >>>> + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " >>>> + "must have initial buffer size (%d) < " >>>> + "buffer size (%"PRId64").\n", >>>> + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); >>>> + return AVERROR(EINVAL); >>>> + } >>>> hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; >>>> - else >>>> + } else { >>>> hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; >>>> + } >>>> + >>>> + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { >>>> + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have >>>> " >>>> + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", >>>> + avctx->bit_rate, avctx->rc_max_rate); >>>> + return AVERROR(EINVAL); >>>> + } >>>> + >>>> + if (avctx->rc_max_rate > avctx->bit_rate) { >>>> + if (!(rc_attr.value & VA_RC_VBR)) { >>>> + av_log(avctx, AV_LOG_WARNING, "Driver does not support " >>>> + "VBR mode (%#x), using CBR mode instead.\n", >>>> + rc_attr.value); >>>> + ctx->va_rc_mode = VA_RC_CBR; >>>> + } else { >>>> + ctx->va_rc_mode = VA_RC_VBR; >>>> + } >>>> + >>>> + rc_bits_per_second = avctx->rc_max_rate; >>>> + rc_target_percentage = (avctx->bit_rate * 100) / avctx- >>>>> rc_max_rate; >>> >>> I think rc_target_percentage should be 100 for CBR case. >> >> Yes; fixed. >> >> The rc_bits_per_second value is also wrong in that case (shouldn't be >> rc_max_rate if we can't use it), fixed similarly. >> >>>> + >>>> + } else if (avctx->rc_max_rate == avctx->bit_rate) { >>>> + if (!(rc_attr.value & VA_RC_CBR)) { >>>> + av_log(avctx, AV_LOG_WARNING, "Driver does not support " >>>> + "CBR mode (%#x), using VBR mode instead.\n", >>>> + rc_attr.value); >>>> + ctx->va_rc_mode = VA_RC_VBR; >>>> + } else { >>>> + ctx->va_rc_mode = VA_RC_CBR; >>>> + } >>>> >>>> - if (ctx->va_rc_mode == VA_RC_CBR) { >>>> rc_bits_per_second = avctx->bit_rate; >>>> rc_target_percentage = 100; >>>> - rc_window_size = 1000; >>>> + >>>> } else { >>>> - if (avctx->rc_max_rate < avctx->bit_rate) { >>>> - // Max rate is unset or invalid, just use the normal bitrate. >>>> + if (rc_attr.value & VA_RC_VBR) { >>>> + ctx->va_rc_mode = VA_RC_VBR; >>> >>> Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR is >>> supported >>> by driver? >> >> I don't think so? VBR with the specified target is probably what you want in >> most cases, and I think anyone with specific constraints that want constant >> bitrate should expect to set maxrate to achieve that. >> > > I agree VBR is probably what an user wants in most case, however target percent > set to 50% is not suitable for most case. To get a specific target percent, user > should set both target bitrate and max bitrate, so it is reasonable to ask user > must set both target bitrate and max bitrate for VBR cases, and for CBR user may > set target bitrate only. > > I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to keep > consistency with QSV for this case. Max bitrate has to be set because of the API limitations (because the target bitrate is an integer percentage of max bitrate rather than a standalone value as found in pretty much every other API). Here we'd like to say "no max bitrate constraint", but that isn't possible. I picked 50% because it's high enough to be mostly unconstrained, while not making numbers too much larger so that you might run into rounding or overflow issues. (E.g. picking 1% here might feel better somehow, but that overflows the 2^32 limit at only 40Mbps.) If you prefer different settings for this case then can you explain what they would be? >>>> + >>>> + // We only have a target bitrate, but VAAPI requires that a >>>> + // maximum rate be supplied as well. Since the user has >>>> + // offered no particular constraint, arbitrarily pick a >>>> + // maximum rate of double the target rate. >>>> + rc_bits_per_second = 2 * avctx->bit_rate; >>>> + rc_target_percentage = 50; >>>> + } else { >>>> + ctx->va_rc_mode = VA_RC_CBR; >>>> + >>>> rc_bits_per_second = avctx->bit_rate; >>>> rc_target_percentage = 100; >>>> - } else { >>>> - rc_bits_per_second = avctx->rc_max_rate; >>>> - rc_target_percentage = (avctx->bit_rate * 100) / >>>> rc_bits_per_second; >>>> } >>>> - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; >>>> } >>>> >>>> + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; >>>> + >>>> + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " >>>> + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", >>>> + rc_target_percentage, rc_bits_per_second, rc_window_size); >>>> + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " >>>> + "initial fullness %"PRId64" bits.\n", >>>> + hrd_buffer_size, hrd_initial_buffer_fullness); >>>> + >>>> + if (rc_bits_per_second > UINT32_MAX || >>>> + hrd_buffer_size > UINT32_MAX || >>>> + hrd_initial_buffer_fullness > UINT32_MAX) { >>>> + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " >>>> + "greater are not supported by VAAPI.\n"); >>>> + return AVERROR(EINVAL); >>>> + } >>>> + >>>> + ctx->va_bit_rate = rc_bits_per_second; >>>> + >>>> + ctx->config_attributes[ctx->nb_config_attributes++] = >>>> + (VAConfigAttrib) { >>>> + .type = VAConfigAttribRateControl, >>>> + .value = ctx->va_rc_mode, >>>> + }; >>>> + >>>> ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl; >>>> ctx->rc_params.rc = (VAEncMiscParameterRateControl) { >>>> .bits_per_second = rc_bits_per_second, >>>> @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext >>>> *avctx) >>>> if (err < 0) >>>> goto fail; >>>> >>>> + err = vaapi_encode_init_rate_control(avctx); >>>> + if (err < 0) >>>> + goto fail; >>>> + >>>> err = vaapi_encode_config_attributes(avctx); >>>> if (err < 0) >>>> goto fail; >>>> @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext >>>> *avctx) >>>> goto fail; >>>> } >>>> >>>> - if (ctx->va_rc_mode & ~VA_RC_CQP) { >>>> - err = vaapi_encode_init_rate_control(avctx); >>>> - if (err < 0) >>>> - goto fail; >>>> - } >>>> - >>>> if (ctx->codec->configure) { >>>> err = ctx->codec->configure(avctx); >>>> if (err < 0) >>>> ...
On 14/06/18 08:22, Li, Zhong wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Xiang, Haihao >> Sent: Thursday, June 14, 2018 2:08 PM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate >> control configuration >> >> On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: >>> On 13/06/18 08:03, Xiang, Haihao wrote: >>>> On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: >>>>> Query which modes are supported and select between VBR and CBR >>>>> based on that - this removes all of the codec-specific rate >>>>> control mode selection code. >>>>> --- >>>>> doc/encoders.texi | 2 - >>>>> libavcodec/vaapi_encode.c | 173 >> ++++++++++++++++++++++++++++------- >>>>> ---- >>>>> - >>>>> libavcodec/vaapi_encode.h | 6 +- >>>>> libavcodec/vaapi_encode_h264.c | 18 +---- >>>>> libavcodec/vaapi_encode_h265.c | 14 +--- >>>>> libavcodec/vaapi_encode_mjpeg.c | 3 +- >>>>> libavcodec/vaapi_encode_mpeg2.c | 9 +-- >>>>> libavcodec/vaapi_encode_vp8.c | 13 +-- >>>>> libavcodec/vaapi_encode_vp9.c | 13 +-- >>>>> 9 files changed, 137 insertions(+), 114 deletions(-) >>>>> >>>>> ... >>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c >>>>> index f4c063734c..5de5483454 100644 >>>>> --- a/libavcodec/vaapi_encode.c >>>>> +++ b/libavcodec/vaapi_encode.c >>>>> ... >>>>> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || >>>>> + avctx->bit_rate <= 0) { This condition ^ >>>>> + if (rc_attr.value & VA_RC_CQP) { >>>>> + av_log(avctx, AV_LOG_VERBOSE, "Using >> constant-quality >>>>> mode.\n"); >>>>> + ctx->va_rc_mode = VA_RC_CQP; >>>>> + return 0; >>>>> + } else { >>>>> + av_log(avctx, AV_LOG_ERROR, "Driver does not >> support " >>>>> + "constant-quality mode (%#x).\n", >> rc_attr.value); >>>>> + return AVERROR(EINVAL); >>>>> + } >>>>> + } >>>>> ... >>>>> + } else if (avctx->rc_max_rate == avctx->bit_rate) { >>>>> + if (!(rc_attr.value & VA_RC_CBR)) { >>>>> + av_log(avctx, AV_LOG_WARNING, "Driver does not >> support " >>>>> + "CBR mode (%#x), using VBR mode >> instead.\n", >>>>> + rc_attr.value); >>>>> + ctx->va_rc_mode = VA_RC_VBR; >>>>> + } else { >>>>> + ctx->va_rc_mode = VA_RC_CBR; >>>>> + } >>>>> >>>>> - if (ctx->va_rc_mode == VA_RC_CBR) { >>>>> rc_bits_per_second = avctx->bit_rate; >>>>> rc_target_percentage = 100; >>>>> - rc_window_size = 1000; >>>>> + >>>>> } else { >>>>> - if (avctx->rc_max_rate < avctx->bit_rate) { >>>>> - // Max rate is unset or invalid, just use the normal >> bitrate. >>>>> + if (rc_attr.value & VA_RC_VBR) { >>>>> + ctx->va_rc_mode = VA_RC_VBR; >>>> >>>> Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR >>>> is supported by driver? >>> >>> I don't think so? VBR with the specified target is probably what you >>> want in most cases, and I think anyone with specific constraints that >>> want constant bitrate should expect to set maxrate to achieve that. >>> >> >> I agree VBR is probably what an user wants in most case, however target >> percent set to 50% is not suitable for most case. To get a specific target >> percent, user should set both target bitrate and max bitrate, so it is >> reasonable to ask user must set both target bitrate and max bitrate for VBR >> cases, and for CBR user may set target bitrate only. > > How about set the max_rate to be a very larger number such as INT_MAX if user hasn't set it? > User may don't set max_rate on purpose, expecting better quality with unlimited bitrate fluctuation (common requirement for local video files). > Double of target_bit_rate is too strict IMHO. And I haven't such a limitation in x264 ABR mode. This unconstrained setup you describe was my intent (as you say, it's usually what you want for local files), but unfortunately the API doesn't really let us do it - the target bitrate is specified as an integer percentage of the max bitrate. 50% was an arbitrary value picked to not have a strong constraint but also not be small enough that we need to think about rounding/overflow problems. We could try to pick a large value with the right properties (for example: if target < 2^32 / 100 then choose maxrate = target * 100, percentage = 1; otherwise choose percentage = 2^32 * 100 / bitrate, maxrate = bitrate * 100 / percentage), but that would be complex to test and I don't think the drivers handle this sort of setup very well. >> I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to keep >> consistency with QSV for this case. > > What will happen if user set a max_rate but without a setting for target_bitrate? > The mode will be VBR (if driver support) with target_bitrate=0. > Tried this on qsv, MSDK returns an error of invalid video parameters. > Is it ok for vaapi? And also with iHD driver? If AVCodecContext.bit_rate isn't set then we use constant-quality mode instead - see the block I've pointed out above. There isn't currently any constant-quality with max-rate constraint mode in VAAPI. >>>>> + >>>>> + // We only have a target bitrate, but VAAPI requires >> that a >>>>> + // maximum rate be supplied as well. Since the user >> has >>>>> + // offered no particular constraint, arbitrarily pick a >>>>> + // maximum rate of double the target rate. >>>>> + rc_bits_per_second = 2 * avctx->bit_rate; >>>>> + rc_target_percentage = 50; >>>>> + } else { >>>>> + ctx->va_rc_mode = VA_RC_CBR; >>>>> + >>>>> rc_bits_per_second = avctx->bit_rate; >>>>> rc_target_percentage = 100; >>>>> - } else { >>>>> - rc_bits_per_second = avctx->rc_max_rate; >>>>> - rc_target_percentage = (avctx->bit_rate * 100) / >>>>> rc_bits_per_second; >>>>> } >>>>> - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; >>>>> } >>>>> ... Thanks, - Mark
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Sunday, June 17, 2018 9:51 PM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate > control configuration > > On 14/06/18 08:22, Li, Zhong wrote: > >> -----Original Message----- > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > >> Of Xiang, Haihao > >> Sent: Thursday, June 14, 2018 2:08 PM > >> To: ffmpeg-devel@ffmpeg.org > >> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up > >> rate control configuration > >> > >> On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: > >>> On 13/06/18 08:03, Xiang, Haihao wrote: > >>>> On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: > >>>>> Query which modes are supported and select between VBR and CBR > >>>>> based on that - this removes all of the codec-specific rate > >>>>> control mode selection code. > >>>>> --- > >>>>> doc/encoders.texi | 2 - > >>>>> libavcodec/vaapi_encode.c | 173 > >> ++++++++++++++++++++++++++++------- > >>>>> ---- > >>>>> - > >>>>> libavcodec/vaapi_encode.h | 6 +- > >>>>> libavcodec/vaapi_encode_h264.c | 18 +---- > >>>>> libavcodec/vaapi_encode_h265.c | 14 +--- > >>>>> libavcodec/vaapi_encode_mjpeg.c | 3 +- > >>>>> libavcodec/vaapi_encode_mpeg2.c | 9 +-- > >>>>> libavcodec/vaapi_encode_vp8.c | 13 +-- > >>>>> libavcodec/vaapi_encode_vp9.c | 13 +-- > >>>>> 9 files changed, 137 insertions(+), 114 deletions(-) > >>>>> > >>>>> ... > >>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > >>>>> index f4c063734c..5de5483454 100644 > >>>>> --- a/libavcodec/vaapi_encode.c > >>>>> +++ b/libavcodec/vaapi_encode.c > >>>>> ... > >>>>> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > >>>>> + avctx->bit_rate <= 0) { > > This condition ^ > > >>>>> + if (rc_attr.value & VA_RC_CQP) { > >>>>> + av_log(avctx, AV_LOG_VERBOSE, "Using > >> constant-quality > >>>>> mode.\n"); > >>>>> + ctx->va_rc_mode = VA_RC_CQP; > >>>>> + return 0; > >>>>> + } else { > >>>>> + av_log(avctx, AV_LOG_ERROR, "Driver does not > >> support " > >>>>> + "constant-quality mode (%#x).\n", > >> rc_attr.value); > >>>>> + return AVERROR(EINVAL); > >>>>> + } > >>>>> + } > >>>>> ... > >>>>> + } else if (avctx->rc_max_rate == avctx->bit_rate) { > >>>>> + if (!(rc_attr.value & VA_RC_CBR)) { > >>>>> + av_log(avctx, AV_LOG_WARNING, "Driver does not > >> support " > >>>>> + "CBR mode (%#x), using VBR mode > >> instead.\n", > >>>>> + rc_attr.value); > >>>>> + ctx->va_rc_mode = VA_RC_VBR; > >>>>> + } else { > >>>>> + ctx->va_rc_mode = VA_RC_CBR; > >>>>> + } > >>>>> > >>>>> - if (ctx->va_rc_mode == VA_RC_CBR) { > >>>>> rc_bits_per_second = avctx->bit_rate; > >>>>> rc_target_percentage = 100; > >>>>> - rc_window_size = 1000; > >>>>> + > >>>>> } else { > >>>>> - if (avctx->rc_max_rate < avctx->bit_rate) { > >>>>> - // Max rate is unset or invalid, just use the normal > >> bitrate. > >>>>> + if (rc_attr.value & VA_RC_VBR) { > >>>>> + ctx->va_rc_mode = VA_RC_VBR; > >>>> > >>>> Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR > >>>> is supported by driver? > >>> > >>> I don't think so? VBR with the specified target is probably what > >>> you want in most cases, and I think anyone with specific constraints > >>> that want constant bitrate should expect to set maxrate to achieve that. > >>> > >> > >> I agree VBR is probably what an user wants in most case, however > >> target percent set to 50% is not suitable for most case. To get a > >> specific target percent, user should set both target bitrate and max > >> bitrate, so it is reasonable to ask user must set both target bitrate > >> and max bitrate for VBR cases, and for CBR user may set target bitrate > only. > > > > How about set the max_rate to be a very larger number such as INT_MAX > if user hasn't set it? > > User may don't set max_rate on purpose, expecting better quality with > unlimited bitrate fluctuation (common requirement for local video files). > > Double of target_bit_rate is too strict IMHO. And I haven't such a > limitation in x264 ABR mode. > > This unconstrained setup you describe was my intent (as you say, it's usually > what you want for local files), but unfortunately the API doesn't really let us > do it - the target bitrate is specified as an integer percentage of the max > bitrate. 50% was an arbitrary value picked to not have a strong constraint > but also not be small enough that we need to think about rounding/overflow > problems. > > We could try to pick a large value with the right properties (for example: if > target < 2^32 / 100 then choose maxrate = target * 100, percentage = 1; > otherwise choose percentage = 2^32 * 100 / bitrate, maxrate = bitrate * 100 > / percentage), but that would be complex to test and I don't think the drivers > handle this sort of setup very well. > > >> I just saw it is also VBR in QSV when max bitrate is not set so I'm > >> fine to keep consistency with QSV for this case. > > > > What will happen if user set a max_rate but without a setting for > target_bitrate? > > The mode will be VBR (if driver support) with target_bitrate=0. > > Tried this on qsv, MSDK returns an error of invalid video parameters. > > Is it ok for vaapi? And also with iHD driver? > > If AVCodecContext.bit_rate isn't set then we use constant-quality mode > instead - see the block I've pointed out above. > > There isn't currently any constant-quality with max-rate constraint mode in > VAAPI. Then the problem I see is that -max_rate hasn't been respected well if user set it (he may don't care about the target bitrate except the peak value). Maybe we can add a warning at least? > > >>>>> + > >>>>> + // We only have a target bitrate, but VAAPI requires > >> that a > >>>>> + // maximum rate be supplied as well. Since the user > >> has > >>>>> + // offered no particular constraint, arbitrarily pick a > >>>>> + // maximum rate of double the target rate. > >>>>> + rc_bits_per_second = 2 * avctx->bit_rate; > >>>>> + rc_target_percentage = 50; > >>>>> + } else { > >>>>> + ctx->va_rc_mode = VA_RC_CBR; > >>>>> + > >>>>> rc_bits_per_second = avctx->bit_rate; > >>>>> rc_target_percentage = 100; > >>>>> - } else { > >>>>> - rc_bits_per_second = avctx->rc_max_rate; > >>>>> - rc_target_percentage = (avctx->bit_rate * 100) / > >>>>> rc_bits_per_second; > >>>>> } > >>>>> - rc_window_size = (hrd_buffer_size * 1000) / > avctx->bit_rate; > >>>>> } > >>>>> ... > > Thanks, > > - Mark
On 20/06/18 10:44, Li, Zhong wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Mark Thompson >> Sent: Sunday, June 17, 2018 9:51 PM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate >> control configuration >> >> On 14/06/18 08:22, Li, Zhong wrote: >>>> -----Original Message----- >>>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On >> Behalf >>>> Of Xiang, Haihao >>>> Sent: Thursday, June 14, 2018 2:08 PM >>>> To: ffmpeg-devel@ffmpeg.org >>>> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up >>>> rate control configuration >>>> >>>> On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: >>>>> On 13/06/18 08:03, Xiang, Haihao wrote: >>>>>> On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: >>>>>>> Query which modes are supported and select between VBR and CBR >>>>>>> based on that - this removes all of the codec-specific rate >>>>>>> control mode selection code. >>>>>>> --- >>>>>>> doc/encoders.texi | 2 - >>>>>>> libavcodec/vaapi_encode.c | 173 >>>> ++++++++++++++++++++++++++++------- >>>>>>> ---- >>>>>>> - >>>>>>> libavcodec/vaapi_encode.h | 6 +- >>>>>>> libavcodec/vaapi_encode_h264.c | 18 +---- >>>>>>> libavcodec/vaapi_encode_h265.c | 14 +--- >>>>>>> libavcodec/vaapi_encode_mjpeg.c | 3 +- >>>>>>> libavcodec/vaapi_encode_mpeg2.c | 9 +-- >>>>>>> libavcodec/vaapi_encode_vp8.c | 13 +-- >>>>>>> libavcodec/vaapi_encode_vp9.c | 13 +-- >>>>>>> 9 files changed, 137 insertions(+), 114 deletions(-) >>>>>>> >>>>>>> ... >>>>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c >>>>>>> index f4c063734c..5de5483454 100644 >>>>>>> --- a/libavcodec/vaapi_encode.c >>>>>>> +++ b/libavcodec/vaapi_encode.c >>>>>>> ... >>>>>>> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || >>>>>>> + avctx->bit_rate <= 0) { >> >> This condition ^ >> >>>>>>> + if (rc_attr.value & VA_RC_CQP) { >>>>>>> + av_log(avctx, AV_LOG_VERBOSE, "Using >>>> constant-quality >>>>>>> mode.\n"); >>>>>>> + ctx->va_rc_mode = VA_RC_CQP; >>>>>>> + return 0; >>>>>>> + } else { >>>>>>> + av_log(avctx, AV_LOG_ERROR, "Driver does not >>>> support " >>>>>>> + "constant-quality mode (%#x).\n", >>>> rc_attr.value); >>>>>>> + return AVERROR(EINVAL); >>>>>>> + } >>>>>>> + } >>>>>>> ... >>>>>>> + } else if (avctx->rc_max_rate == avctx->bit_rate) { >>>>>>> + if (!(rc_attr.value & VA_RC_CBR)) { >>>>>>> + av_log(avctx, AV_LOG_WARNING, "Driver does not >>>> support " >>>>>>> + "CBR mode (%#x), using VBR mode >>>> instead.\n", >>>>>>> + rc_attr.value); >>>>>>> + ctx->va_rc_mode = VA_RC_VBR; >>>>>>> + } else { >>>>>>> + ctx->va_rc_mode = VA_RC_CBR; >>>>>>> + } >>>>>>> >>>>>>> - if (ctx->va_rc_mode == VA_RC_CBR) { >>>>>>> rc_bits_per_second = avctx->bit_rate; >>>>>>> rc_target_percentage = 100; >>>>>>> - rc_window_size = 1000; >>>>>>> + >>>>>>> } else { >>>>>>> - if (avctx->rc_max_rate < avctx->bit_rate) { >>>>>>> - // Max rate is unset or invalid, just use the normal >>>> bitrate. >>>>>>> + if (rc_attr.value & VA_RC_VBR) { >>>>>>> + ctx->va_rc_mode = VA_RC_VBR; >>>>>> >>>>>> Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR >>>>>> is supported by driver? >>>>> >>>>> I don't think so? VBR with the specified target is probably what >>>>> you want in most cases, and I think anyone with specific constraints >>>>> that want constant bitrate should expect to set maxrate to achieve that. >>>>> >>>> >>>> I agree VBR is probably what an user wants in most case, however >>>> target percent set to 50% is not suitable for most case. To get a >>>> specific target percent, user should set both target bitrate and max >>>> bitrate, so it is reasonable to ask user must set both target bitrate >>>> and max bitrate for VBR cases, and for CBR user may set target bitrate >> only. >>> >>> How about set the max_rate to be a very larger number such as INT_MAX >> if user hasn't set it? >>> User may don't set max_rate on purpose, expecting better quality with >> unlimited bitrate fluctuation (common requirement for local video files). >>> Double of target_bit_rate is too strict IMHO. And I haven't such a >> limitation in x264 ABR mode. >> >> This unconstrained setup you describe was my intent (as you say, it's usually >> what you want for local files), but unfortunately the API doesn't really let us >> do it - the target bitrate is specified as an integer percentage of the max >> bitrate. 50% was an arbitrary value picked to not have a strong constraint >> but also not be small enough that we need to think about rounding/overflow >> problems. >> >> We could try to pick a large value with the right properties (for example: if >> target < 2^32 / 100 then choose maxrate = target * 100, percentage = 1; >> otherwise choose percentage = 2^32 * 100 / bitrate, maxrate = bitrate * 100 >> / percentage), but that would be complex to test and I don't think the drivers >> handle this sort of setup very well. >> >>>> I just saw it is also VBR in QSV when max bitrate is not set so I'm >>>> fine to keep consistency with QSV for this case. >>> >>> What will happen if user set a max_rate but without a setting for >> target_bitrate? >>> The mode will be VBR (if driver support) with target_bitrate=0. >>> Tried this on qsv, MSDK returns an error of invalid video parameters. >>> Is it ok for vaapi? And also with iHD driver? >> >> If AVCodecContext.bit_rate isn't set then we use constant-quality mode >> instead - see the block I've pointed out above. >> >> There isn't currently any constant-quality with max-rate constraint mode in >> VAAPI. > > Then the problem I see is that -max_rate hasn't been respected well if user set it (he may don't care about the target bitrate except the peak value). > Maybe we can add a warning at least? Given that it's really CQP, I don't think anyone would ever expect this to work? Encoders generally don't warn about ignoring extra irrelevant options in AVCodecContext. (Is there any encoder at all which supports that combination? E.g. libx264 supports maxrate in CRF but not CQP mode.) - Mark
On Thu, Jun 21, 2018 at 12:10:04AM +0100, Mark Thompson wrote: > On 20/06/18 10:44, Li, Zhong wrote: > >> -----Original Message----- > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > >> Of Mark Thompson > >> Sent: Sunday, June 17, 2018 9:51 PM > >> To: ffmpeg-devel@ffmpeg.org > >> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate > >> control configuration > >> > >> On 14/06/18 08:22, Li, Zhong wrote: > >>>> -----Original Message----- > >>>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > >> Behalf > >>>> Of Xiang, Haihao > >>>> Sent: Thursday, June 14, 2018 2:08 PM > >>>> To: ffmpeg-devel@ffmpeg.org > >>>> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up > >>>> rate control configuration > >>>> > >>>> On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote: > >>>>> On 13/06/18 08:03, Xiang, Haihao wrote: > >>>>>> On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: > >>>>>>> Query which modes are supported and select between VBR and CBR > >>>>>>> based on that - this removes all of the codec-specific rate > >>>>>>> control mode selection code. > >>>>>>> --- > >>>>>>> doc/encoders.texi | 2 - > >>>>>>> libavcodec/vaapi_encode.c | 173 > >>>> ++++++++++++++++++++++++++++------- > >>>>>>> ---- > >>>>>>> - > >>>>>>> libavcodec/vaapi_encode.h | 6 +- > >>>>>>> libavcodec/vaapi_encode_h264.c | 18 +---- > >>>>>>> libavcodec/vaapi_encode_h265.c | 14 +--- > >>>>>>> libavcodec/vaapi_encode_mjpeg.c | 3 +- > >>>>>>> libavcodec/vaapi_encode_mpeg2.c | 9 +-- > >>>>>>> libavcodec/vaapi_encode_vp8.c | 13 +-- > >>>>>>> libavcodec/vaapi_encode_vp9.c | 13 +-- > >>>>>>> 9 files changed, 137 insertions(+), 114 deletions(-) > >>>>>>> > >>>>>>> ... > >>>>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > >>>>>>> index f4c063734c..5de5483454 100644 > >>>>>>> --- a/libavcodec/vaapi_encode.c > >>>>>>> +++ b/libavcodec/vaapi_encode.c > >>>>>>> ... > >>>>>>> + if (avctx->flags & AV_CODEC_FLAG_QSCALE || > >>>>>>> + avctx->bit_rate <= 0) { > >> > >> This condition ^ > >> > >>>>>>> + if (rc_attr.value & VA_RC_CQP) { > >>>>>>> + av_log(avctx, AV_LOG_VERBOSE, "Using > >>>> constant-quality > >>>>>>> mode.\n"); > >>>>>>> + ctx->va_rc_mode = VA_RC_CQP; > >>>>>>> + return 0; > >>>>>>> + } else { > >>>>>>> + av_log(avctx, AV_LOG_ERROR, "Driver does not > >>>> support " > >>>>>>> + "constant-quality mode (%#x).\n", > >>>> rc_attr.value); > >>>>>>> + return AVERROR(EINVAL); > >>>>>>> + } > >>>>>>> + } > >>>>>>> ... > >>>>>>> + } else if (avctx->rc_max_rate == avctx->bit_rate) { > >>>>>>> + if (!(rc_attr.value & VA_RC_CBR)) { > >>>>>>> + av_log(avctx, AV_LOG_WARNING, "Driver does not > >>>> support " > >>>>>>> + "CBR mode (%#x), using VBR mode > >>>> instead.\n", > >>>>>>> + rc_attr.value); > >>>>>>> + ctx->va_rc_mode = VA_RC_VBR; > >>>>>>> + } else { > >>>>>>> + ctx->va_rc_mode = VA_RC_CBR; > >>>>>>> + } > >>>>>>> > >>>>>>> - if (ctx->va_rc_mode == VA_RC_CBR) { > >>>>>>> rc_bits_per_second = avctx->bit_rate; > >>>>>>> rc_target_percentage = 100; > >>>>>>> - rc_window_size = 1000; > >>>>>>> + > >>>>>>> } else { > >>>>>>> - if (avctx->rc_max_rate < avctx->bit_rate) { > >>>>>>> - // Max rate is unset or invalid, just use the normal > >>>> bitrate. > >>>>>>> + if (rc_attr.value & VA_RC_VBR) { > >>>>>>> + ctx->va_rc_mode = VA_RC_VBR; > >>>>>> > >>>>>> Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR > >>>>>> is supported by driver? > >>>>> > >>>>> I don't think so? VBR with the specified target is probably what > >>>>> you want in most cases, and I think anyone with specific constraints > >>>>> that want constant bitrate should expect to set maxrate to achieve that. > >>>>> > >>>> > >>>> I agree VBR is probably what an user wants in most case, however > >>>> target percent set to 50% is not suitable for most case. To get a > >>>> specific target percent, user should set both target bitrate and max > >>>> bitrate, so it is reasonable to ask user must set both target bitrate > >>>> and max bitrate for VBR cases, and for CBR user may set target bitrate > >> only. > >>> > >>> How about set the max_rate to be a very larger number such as INT_MAX > >> if user hasn't set it? > >>> User may don't set max_rate on purpose, expecting better quality with > >> unlimited bitrate fluctuation (common requirement for local video files). > >>> Double of target_bit_rate is too strict IMHO. And I haven't such a > >> limitation in x264 ABR mode. > >> > >> This unconstrained setup you describe was my intent (as you say, it's usually > >> what you want for local files), but unfortunately the API doesn't really let us > >> do it - the target bitrate is specified as an integer percentage of the max > >> bitrate. 50% was an arbitrary value picked to not have a strong constraint > >> but also not be small enough that we need to think about rounding/overflow > >> problems. > >> > >> We could try to pick a large value with the right properties (for example: if > >> target < 2^32 / 100 then choose maxrate = target * 100, percentage = 1; > >> otherwise choose percentage = 2^32 * 100 / bitrate, maxrate = bitrate * 100 > >> / percentage), but that would be complex to test and I don't think the drivers > >> handle this sort of setup very well. > >> > >>>> I just saw it is also VBR in QSV when max bitrate is not set so I'm > >>>> fine to keep consistency with QSV for this case. > >>> > >>> What will happen if user set a max_rate but without a setting for > >> target_bitrate? > >>> The mode will be VBR (if driver support) with target_bitrate=0. > >>> Tried this on qsv, MSDK returns an error of invalid video parameters. > >>> Is it ok for vaapi? And also with iHD driver? > >> > >> If AVCodecContext.bit_rate isn't set then we use constant-quality mode > >> instead - see the block I've pointed out above. > >> > >> There isn't currently any constant-quality with max-rate constraint mode in > >> VAAPI. > > > > Then the problem I see is that -max_rate hasn't been respected well if user set it (he may don't care about the target bitrate except the peak value). > > Maybe we can add a warning at least? > > Given that it's really CQP, I don't think anyone would ever expect this to work? Encoders generally don't warn about ignoring extra irrelevant options in AVCodecContext. > > (Is there any encoder at all which supports that combination? E.g. libx264 supports maxrate in CRF but not CQP mode.) if i understand correctly, then yes, see vbv_ignore_qmax. If max rate cannot be achievied the mpegvideo encoders should attempt to encode the frame again without qmax and at lower quality [...]
diff --git a/doc/encoders.texi b/doc/encoders.texi index 62a1509a96..0c0a307987 100644 --- a/doc/encoders.texi +++ b/doc/encoders.texi @@ -2643,8 +2643,6 @@ Always encodes using the standard quantisation and huffman tables - @item mpeg2_vaapi @option{profile} and @option{level} set the value of @emph{profile_and_level_indication}. -No rate control is supported. - @item vp8_vaapi B-frames are not supported. diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c index f4c063734c..5de5483454 100644 --- a/libavcodec/vaapi_encode.c +++ b/libavcodec/vaapi_encode.c @@ -1217,7 +1217,6 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) int i; VAConfigAttrib attr[] = { - { VAConfigAttribRateControl }, { VAConfigAttribEncMaxRefFrames }, { VAConfigAttribEncPackedHeaders }, }; @@ -1241,32 +1240,6 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) continue; } switch (attr[i].type) { - case VAConfigAttribRateControl: - // Hack for backward compatibility: CBR was the only - // usable RC mode for a long time, so old drivers will - // only have it. Normal default options may now choose - // VBR and then fail, however, so override it here with - // CBR if that is the only supported mode. - if (ctx->va_rc_mode == VA_RC_VBR && - !(attr[i].value & VA_RC_VBR) && - (attr[i].value & VA_RC_CBR)) { - av_log(avctx, AV_LOG_WARNING, "VBR rate control is " - "not supported with this driver version; " - "using CBR instead.\n"); - ctx->va_rc_mode = VA_RC_CBR; - } - if (!(ctx->va_rc_mode & attr[i].value)) { - av_log(avctx, AV_LOG_ERROR, "Rate control mode %#x " - "is not supported (mask: %#x).\n", - ctx->va_rc_mode, attr[i].value); - return AVERROR(EINVAL); - } - ctx->config_attributes[ctx->nb_config_attributes++] = - (VAConfigAttrib) { - .type = VAConfigAttribRateControl, - .value = ctx->va_rc_mode, - }; - break; case VAConfigAttribEncMaxRefFrames: { unsigned int ref_l0 = attr[i].value & 0xffff; @@ -1313,44 +1286,144 @@ static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx) static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) { VAAPIEncodeContext *ctx = avctx->priv_data; - int rc_bits_per_second; - int rc_target_percentage; - int rc_window_size; - int hrd_buffer_size; - int hrd_initial_buffer_fullness; + int64_t rc_bits_per_second; + int rc_target_percentage; + int rc_window_size; + int64_t hrd_buffer_size; + int64_t hrd_initial_buffer_fullness; int fr_num, fr_den; + VAConfigAttrib rc_attr = { VAConfigAttribRateControl }; + VAStatus vas; + + vas = vaGetConfigAttributes(ctx->hwctx->display, + ctx->va_profile, ctx->va_entrypoint, + &rc_attr, 1); + if (vas != VA_STATUS_SUCCESS) { + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control " + "config attribute: %d (%s).\n", vas, vaErrorStr(vas)); + return AVERROR_EXTERNAL; + } - if (avctx->bit_rate > INT32_MAX) { - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or " - "higher is not supported.\n"); + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) { + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any " + "supported rate control modes: assuming constant-quality.\n"); + ctx->va_rc_mode = VA_RC_CQP; + return 0; + } + if (avctx->flags & AV_CODEC_FLAG_QSCALE || + avctx->bit_rate <= 0) { + if (rc_attr.value & VA_RC_CQP) { + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality mode.\n"); + ctx->va_rc_mode = VA_RC_CQP; + return 0; + } else { + av_log(avctx, AV_LOG_ERROR, "Driver does not support " + "constant-quality mode (%#x).\n", rc_attr.value); + return AVERROR(EINVAL); + } + } + + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) { + av_log(avctx, AV_LOG_ERROR, "Driver does not support any " + "bitrate-targetted rate control modes.\n"); return AVERROR(EINVAL); } if (avctx->rc_buffer_size) hrd_buffer_size = avctx->rc_buffer_size; + else if (avctx->rc_max_rate > 0) + hrd_buffer_size = avctx->rc_max_rate; else hrd_buffer_size = avctx->bit_rate; - if (avctx->rc_initial_buffer_occupancy) + if (avctx->rc_initial_buffer_occupancy) { + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) { + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: " + "must have initial buffer size (%d) < " + "buffer size (%"PRId64").\n", + avctx->rc_initial_buffer_occupancy, hrd_buffer_size); + return AVERROR(EINVAL); + } hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy; - else + } else { hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4; + } + + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) { + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have " + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n", + avctx->bit_rate, avctx->rc_max_rate); + return AVERROR(EINVAL); + } + + if (avctx->rc_max_rate > avctx->bit_rate) { + if (!(rc_attr.value & VA_RC_VBR)) { + av_log(avctx, AV_LOG_WARNING, "Driver does not support " + "VBR mode (%#x), using CBR mode instead.\n", + rc_attr.value); + ctx->va_rc_mode = VA_RC_CBR; + } else { + ctx->va_rc_mode = VA_RC_VBR; + } + + rc_bits_per_second = avctx->rc_max_rate; + rc_target_percentage = (avctx->bit_rate * 100) / avctx->rc_max_rate; + + } else if (avctx->rc_max_rate == avctx->bit_rate) { + if (!(rc_attr.value & VA_RC_CBR)) { + av_log(avctx, AV_LOG_WARNING, "Driver does not support " + "CBR mode (%#x), using VBR mode instead.\n", + rc_attr.value); + ctx->va_rc_mode = VA_RC_VBR; + } else { + ctx->va_rc_mode = VA_RC_CBR; + } - if (ctx->va_rc_mode == VA_RC_CBR) { rc_bits_per_second = avctx->bit_rate; rc_target_percentage = 100; - rc_window_size = 1000; + } else { - if (avctx->rc_max_rate < avctx->bit_rate) { - // Max rate is unset or invalid, just use the normal bitrate. + if (rc_attr.value & VA_RC_VBR) { + ctx->va_rc_mode = VA_RC_VBR; + + // We only have a target bitrate, but VAAPI requires that a + // maximum rate be supplied as well. Since the user has + // offered no particular constraint, arbitrarily pick a + // maximum rate of double the target rate. + rc_bits_per_second = 2 * avctx->bit_rate; + rc_target_percentage = 50; + } else { + ctx->va_rc_mode = VA_RC_CBR; + rc_bits_per_second = avctx->bit_rate; rc_target_percentage = 100; - } else { - rc_bits_per_second = avctx->rc_max_rate; - rc_target_percentage = (avctx->bit_rate * 100) / rc_bits_per_second; } - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate; } + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second; + + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps " + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR", + rc_target_percentage, rc_bits_per_second, rc_window_size); + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, " + "initial fullness %"PRId64" bits.\n", + hrd_buffer_size, hrd_initial_buffer_fullness); + + if (rc_bits_per_second > UINT32_MAX || + hrd_buffer_size > UINT32_MAX || + hrd_initial_buffer_fullness > UINT32_MAX) { + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or " + "greater are not supported by VAAPI.\n"); + return AVERROR(EINVAL); + } + + ctx->va_bit_rate = rc_bits_per_second; + + ctx->config_attributes[ctx->nb_config_attributes++] = + (VAConfigAttrib) { + .type = VAConfigAttribRateControl, + .value = ctx->va_rc_mode, + }; + ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl; ctx->rc_params.rc = (VAEncMiscParameterRateControl) { .bits_per_second = rc_bits_per_second, @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) if (err < 0) goto fail; + err = vaapi_encode_init_rate_control(avctx); + if (err < 0) + goto fail; + err = vaapi_encode_config_attributes(avctx); if (err < 0) goto fail; @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx) goto fail; } - if (ctx->va_rc_mode & ~VA_RC_CQP) { - err = vaapi_encode_init_rate_control(avctx); - if (err < 0) - goto fail; - } - if (ctx->codec->configure) { err = ctx->codec->configure(avctx); if (err < 0) diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h index b8f2ed6d21..6fac4781c3 100644 --- a/libavcodec/vaapi_encode.h +++ b/libavcodec/vaapi_encode.h @@ -116,8 +116,6 @@ typedef struct VAAPIEncodeContext { // Use low power encoding mode. int low_power; - // Rate control mode. - unsigned int va_rc_mode; // Supported packed headers (initially the desired set, modified // later to what is actually supported). unsigned int va_packed_headers; @@ -138,6 +136,10 @@ typedef struct VAAPIEncodeContext { VAProfile va_profile; // Encoding entrypoint (VAEntryoint*). VAEntrypoint va_entrypoint; + // Rate control mode. + unsigned int va_rc_mode; + // Bitrate for codec-specific encoder parameters. + unsigned int va_bit_rate; // Configuration attributes to use when creating va_config. VAConfigAttrib config_attributes[MAX_CONFIG_ATTRIBUTES]; diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c index df6f39a946..1e6eb2e39b 100644 --- a/libavcodec/vaapi_encode_h264.c +++ b/libavcodec/vaapi_encode_h264.c @@ -425,9 +425,9 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) // Try to scale these to a sensible range so that the // golomb encode of the value is not overlong. hrd->bit_rate_scale = - av_clip_uintp2(av_log2(avctx->bit_rate) - 15 - 6, 4); + av_clip_uintp2(av_log2(ctx->va_bit_rate) - 15 - 6, 4); hrd->bit_rate_value_minus1[0] = - (avctx->bit_rate >> hrd->bit_rate_scale + 6) - 1; + (ctx->va_bit_rate >> hrd->bit_rate_scale + 6) - 1; hrd->cpb_size_scale = av_clip_uintp2(av_log2(ctx->hrd_params.hrd.buffer_size) - 15 - 4, 4); @@ -497,7 +497,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) .intra_idr_period = avctx->gop_size, .ip_period = ctx->b_per_p + 1, - .bits_per_second = avctx->bit_rate, + .bits_per_second = ctx->va_bit_rate, .max_num_ref_frames = sps->max_num_ref_frames, .picture_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1, .picture_height_in_mbs = sps->pic_height_in_map_units_minus1 + 1, @@ -823,10 +823,6 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx) priv->fixed_qp_p = 26; priv->fixed_qp_b = 26; - av_log(avctx, AV_LOG_DEBUG, "Using %s-bitrate = %"PRId64" bps.\n", - ctx->va_rc_mode == VA_RC_CBR ? "constant" : "variable", - avctx->bit_rate); - } else { av_assert0(0 && "Invalid RC mode."); } @@ -934,14 +930,6 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx) return AVERROR_PATCHWELCOME; } - if (avctx->bit_rate > 0) { - if (avctx->rc_max_rate == avctx->bit_rate) - ctx->va_rc_mode = VA_RC_CBR; - else - ctx->va_rc_mode = VA_RC_VBR; - } else - ctx->va_rc_mode = VA_RC_CQP; - ctx->va_packed_headers = VA_ENC_PACKED_HEADER_SEQUENCE | // SPS and PPS. VA_ENC_PACKED_HEADER_SLICE | // Slice headers. diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c index b8b66b87cb..b296919b37 100644 --- a/libavcodec/vaapi_encode_h265.c +++ b/libavcodec/vaapi_encode_h265.c @@ -512,7 +512,7 @@ static int vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx) .intra_period = avctx->gop_size, .intra_idr_period = avctx->gop_size, .ip_period = ctx->b_per_p + 1, - .bits_per_second = avctx->bit_rate, + .bits_per_second = ctx->va_bit_rate, .pic_width_in_luma_samples = sps->pic_width_in_luma_samples, .pic_height_in_luma_samples = sps->pic_height_in_luma_samples, @@ -1014,10 +1014,6 @@ static av_cold int vaapi_encode_h265_configure(AVCodecContext *avctx) priv->fixed_qp_p = 30; priv->fixed_qp_b = 30; - av_log(avctx, AV_LOG_DEBUG, "Using %s-bitrate = %"PRId64" bps.\n", - ctx->va_rc_mode == VA_RC_CBR ? "constant" : "variable", - avctx->bit_rate); - } else { av_assert0(0 && "Invalid RC mode."); } @@ -1068,14 +1064,6 @@ static av_cold int vaapi_encode_h265_init(AVCodecContext *avctx) if (avctx->level == FF_LEVEL_UNKNOWN) avctx->level = priv->level; - if (avctx->bit_rate > 0) { - if (avctx->rc_max_rate == avctx->bit_rate) - ctx->va_rc_mode = VA_RC_CBR; - else - ctx->va_rc_mode = VA_RC_VBR; - } else - ctx->va_rc_mode = VA_RC_CQP; - ctx->va_packed_headers = VA_ENC_PACKED_HEADER_SEQUENCE | // VPS, SPS and PPS. VA_ENC_PACKED_HEADER_SLICE | // Slice headers. diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c index b328beaa09..67ac2fba96 100644 --- a/libavcodec/vaapi_encode_mjpeg.c +++ b/libavcodec/vaapi_encode_mjpeg.c @@ -388,8 +388,6 @@ static av_cold int vaapi_encode_mjpeg_init(AVCodecContext *avctx) ctx->codec = &vaapi_encode_type_mjpeg; - ctx->va_rc_mode = VA_RC_CQP; - // The JPEG image header - see note above. ctx->va_packed_headers = VA_ENC_PACKED_HEADER_RAW_DATA; @@ -402,6 +400,7 @@ static av_cold int vaapi_encode_mjpeg_init(AVCodecContext *avctx) static const AVCodecDefault vaapi_encode_mjpeg_defaults[] = { { "global_quality", "80" }, + { "b", "0" }, { NULL }, }; diff --git a/libavcodec/vaapi_encode_mpeg2.c b/libavcodec/vaapi_encode_mpeg2.c index db79d72115..ff86b8817e 100644 --- a/libavcodec/vaapi_encode_mpeg2.c +++ b/libavcodec/vaapi_encode_mpeg2.c @@ -188,8 +188,8 @@ static int vaapi_encode_mpeg2_init_sequence_params(AVCodecContext *avctx) memset(pce, 0, sizeof(*pce)); - if (avctx->bit_rate > 0) { - priv->bit_rate = (avctx->bit_rate + 399) / 400; + if (ctx->va_bit_rate > 0) { + priv->bit_rate = (ctx->va_bit_rate + 399) / 400; } else { // Unknown (not a bitrate-targetting mode), so just use the // highest value. @@ -361,7 +361,7 @@ static int vaapi_encode_mpeg2_init_sequence_params(AVCodecContext *avctx) .picture_width = avctx->width, .picture_height = avctx->height, - .bits_per_second = avctx->bit_rate, + .bits_per_second = ctx->va_bit_rate, .frame_rate = av_q2d(priv->frame_rate), .aspect_ratio_information = sh->aspect_ratio_information, .vbv_buffer_size = priv->vbv_buffer_size, @@ -615,8 +615,6 @@ static av_cold int vaapi_encode_mpeg2_init(AVCodecContext *avctx) return AVERROR(EINVAL); } - ctx->va_rc_mode = VA_RC_CQP; - ctx->va_packed_headers = VA_ENC_PACKED_HEADER_SEQUENCE | VA_ENC_PACKED_HEADER_PICTURE; @@ -666,6 +664,7 @@ static const AVOption vaapi_encode_mpeg2_options[] = { }; static const AVCodecDefault vaapi_encode_mpeg2_defaults[] = { + { "b", "0" }, { "bf", "1" }, { "g", "120" }, { "i_qfactor", "1" }, diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c index 9588826bfb..40871a6bbf 100644 --- a/libavcodec/vaapi_encode_vp8.c +++ b/libavcodec/vaapi_encode_vp8.c @@ -65,7 +65,7 @@ static int vaapi_encode_vp8_init_sequence_params(AVCodecContext *avctx) vseq->kf_auto = 0; if (!(ctx->va_rc_mode & VA_RC_CQP)) { - vseq->bits_per_second = avctx->bit_rate; + vseq->bits_per_second = ctx->va_bit_rate; vseq->intra_period = avctx->gop_size; } @@ -205,17 +205,6 @@ static av_cold int vaapi_encode_vp8_init(AVCodecContext *avctx) ctx->codec = &vaapi_encode_type_vp8; - if (avctx->flags & AV_CODEC_FLAG_QSCALE) { - ctx->va_rc_mode = VA_RC_CQP; - } else if (avctx->bit_rate > 0) { - if (avctx->rc_max_rate == avctx->bit_rate) - ctx->va_rc_mode = VA_RC_CBR; - else - ctx->va_rc_mode = VA_RC_VBR; - } else { - ctx->va_rc_mode = VA_RC_CQP; - } - // Packed headers are not currently supported. ctx->va_packed_headers = 0; diff --git a/libavcodec/vaapi_encode_vp9.c b/libavcodec/vaapi_encode_vp9.c index 4d7cec0520..e400bc8b79 100644 --- a/libavcodec/vaapi_encode_vp9.c +++ b/libavcodec/vaapi_encode_vp9.c @@ -71,7 +71,7 @@ static int vaapi_encode_vp9_init_sequence_params(AVCodecContext *avctx) vseq->kf_auto = 0; if (!(ctx->va_rc_mode & VA_RC_CQP)) { - vseq->bits_per_second = avctx->bit_rate; + vseq->bits_per_second = ctx->va_bit_rate; vseq->intra_period = avctx->gop_size; } @@ -227,17 +227,6 @@ static av_cold int vaapi_encode_vp9_init(AVCodecContext *avctx) ctx->codec = &vaapi_encode_type_vp9; - if (avctx->flags & AV_CODEC_FLAG_QSCALE) { - ctx->va_rc_mode = VA_RC_CQP; - } else if (avctx->bit_rate > 0) { - if (avctx->bit_rate == avctx->rc_max_rate) - ctx->va_rc_mode = VA_RC_CBR; - else - ctx->va_rc_mode = VA_RC_VBR; - } else { - ctx->va_rc_mode = VA_RC_CQP; - } - // Packed headers are not currently supported. ctx->va_packed_headers = 0;