On 28/01/2019 02:19, Fu, Linjie wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
>> Of Mark Thompson
>> Sent: Monday, January 28, 2019 07:47
>> To: ffmpeg-devel@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
