[FFmpeg-devel] [PATCH v2 01/11] vaapi_encode: Support more RC modes

Mark Thompson sw at jkqxz.net
Mon Feb 4 11:23:22 EET 2019


On 28/01/2019 02:19, Fu, Linjie wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
>> Of Mark Thompson
>> Sent: Monday, January 28, 2019 07:47
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH v2 01/11] vaapi_encode: Support more RC
>> modes
>>
>> Allow setting the mode explicitly, and try to make a sensible choice
>> given the available parameters if not.
>> ---
>>  doc/encoders.texi         |  24 +++
>>  libavcodec/vaapi_encode.c | 370 +++++++++++++++++++++++++++---------
>> --
>>  libavcodec/vaapi_encode.h |  65 +++++++
>>  3 files changed, 351 insertions(+), 108 deletions(-)
>>
>> ...
>> +
>>  static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>  {
>>      VAAPIEncodeContext *ctx = avctx->priv_data;
>> +    uint32_t supported_va_rc_modes;
>> +    const VAAPIEncodeRCMode *rc_mode;
>>      int64_t rc_bits_per_second;
>>      int     rc_target_percentage;
>>      int     rc_window_size;
>> +    int     rc_quality;
>>      int64_t hrd_buffer_size;
>>      int64_t hrd_initial_buffer_fullness;
>>      int fr_num, fr_den;
>>      VAConfigAttrib rc_attr = { VAConfigAttribRateControl };
>>      VAStatus vas;
>> +    char supported_rc_modes_string[64];
>>
>>      vas = vaGetConfigAttributes(ctx->hwctx->display,
>>                                  ctx->va_profile, ctx->va_entrypoint,
>> @@ -1303,119 +1328,215 @@ static av_cold int
>> vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>                 "config attribute: %d (%s).\n", vas, vaErrorStr(vas));
>>          return AVERROR_EXTERNAL;
>>      }
>> -
>>      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 (ctx->codec->flags & FLAG_CONSTANT_QUALITY_ONLY ||
>> -        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;
>> -            if (avctx->bit_rate > 0 || avctx->rc_max_rate > 0) {
>> -                av_log(avctx, AV_LOG_WARNING, "Bitrate target parameters "
>> -                       "ignored in constant-quality mode.\n");
>> +               "supported rate control modes: assuming CQP only.\n");
>> +        supported_va_rc_modes = VA_RC_CQP;
>> +        strcpy(supported_rc_modes_string, "unknown");
>> +    } else {
>> +        char *str = supported_rc_modes_string;
>> +        size_t len = sizeof(supported_rc_modes_string);
>> +        int i, first = 1, res;
>> +
>> +        supported_va_rc_modes = rc_attr.value;
>> +
>> +        first = 1;
> 
> Redundant “first” here I think.

Yep, removed.

>> +        for (i = 0; i < FF_ARRAY_ELEMS(vaapi_encode_rc_modes); i++) {
>> +            rc_mode = &vaapi_encode_rc_modes[i];
>> +            if (supported_va_rc_modes & rc_mode->va_mode) {
>> +                res = snprintf(str, len, "%s%s",
>> +                               first ? "" : ", ", rc_mode->name);
>> +                first = 0;
>> +                if (res < 0) {
>> +                    *str = 0;
>> +                    break;
>> +                }
>> +                len -= res;
>> +                str += res;
>> +                if (len == 0)
>> +                    break;
>>              }
>> -            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);
>> +        av_log(avctx, AV_LOG_DEBUG, "Driver supports RC modes %s.\n",
>> +               supported_rc_modes_string);
>> +    }
>> +
>> +    // Rate control mode selection:
>> +    // * If the user has set a mode explicitly with the rc_mode option,
>> +    //   use it and fail if it is not available.
>> +    // * If an explicit QP option has been set, use CQP.
>> +    // * If the codec is CQ-only, use CQP.
>> +    // * If the QSCALE avcodec option is set, use CQP.
>> +    // * If bitrate and quality are both set, try QVBR.
>> +    // * If quality is set, try ICQ, then CQP.
>> +    // * If bitrate and maxrate are set and have the same value, try CBR.
>> +    // * If a bitrate is set, try AVBR, then VBR, then CBR.
>> +    // * If no bitrate is set, try ICQ, then CQP.
>> +
>> +#define TRY_RC_MODE(mode, fail) do { \
>> +        rc_mode = &vaapi_encode_rc_modes[mode]; \
>> +        if (!(rc_mode->va_mode & supported_va_rc_modes)) { \
>> +            if (fail) { \
>> +                av_log(avctx, AV_LOG_ERROR, "Driver does not support %s " \
>> +                       "RC mode (supported modes: %s).\n", rc_mode->name, \
>> +                       supported_rc_modes_string); \
>> +                return AVERROR(EINVAL); \
>> +            } \
>> +            av_log(avctx, AV_LOG_DEBUG, "Driver does not support %s " \
>> +                   "RC mode.\n", rc_mode->name); \
>> +            rc_mode = NULL; \
>> +        } else { \
>> +            goto rc_mode_found; \
>> +        } \
>> +    } while (0)
>> +
> 
> Will it be better to put the definition of TRY_RC_MODE in the front of this file?

I don't think so?  Putting it here makes it clear that it is function-local, using jumps and stack variables which wouldn't be accessible outside the function.  It's also right next to all of the uses, so it's easy to see how any given use will expand.  Since it is local it probably should be #undef'ed at the end of the block using it, though, so I've added that.

Thanks,

- Mark


More information about the ffmpeg-devel mailing list