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)
>>>> ...
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
