On 21/06/18 18:03, Michael Niedermayer wrote: > 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-boun...@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-boun...@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
Ok, fair enough. I've added a warning as below so that it's clear this case isn't supported. diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c index 53c3a2132a..25d89c65c9 100644 --- a/libavcodec/vaapi_encode.c +++ b/libavcodec/vaapi_encode.c @@ -1311,6 +1311,10 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx) 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"); + } return 0; } else { av_log(avctx, AV_LOG_ERROR, "Driver does not support " Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel