Re: [FFmpeg-devel] [PATCH v5 4/9] avcodec/vaapi_encode: extract rc parameter configuration to base layer

["Wu, Tong1" <tong1.wu-at-intel.com@ffmpeg.org>]

>On 18/02/2024 08:45, tong1.wu-at-intel.com@ffmpeg.org wrote:
>> From: Tong Wu <tong1.wu@intel.com>
>>
>> VAAPI and D3D12VA can share rate control configuration codes. Hence, it
>> can be moved to base layer for simplification.
>>
>> Signed-off-by: Tong Wu <tong1.wu@intel.com>
>> ---
>>   libavcodec/hw_base_encode.c    | 151 ++++++++++++++++++++++++
>>   libavcodec/hw_base_encode.h    |  34 ++++++
>>   libavcodec/vaapi_encode.c      | 210 ++++++---------------------------
>>   libavcodec/vaapi_encode.h      |  14 +--
>>   libavcodec/vaapi_encode_av1.c  |   2 +-
>>   libavcodec/vaapi_encode_h264.c |   2 +-
>>   libavcodec/vaapi_encode_vp9.c  |   2 +-
>>   7 files changed, 227 insertions(+), 188 deletions(-)
>>
>> diff --git a/libavcodec/hw_base_encode.c b/libavcodec/hw_base_encode.c
>> index f0e4ef9655..c20c47bf55 100644
>> --- a/libavcodec/hw_base_encode.c
>> +++ b/libavcodec/hw_base_encode.c
>> @@ -631,6 +631,157 @@ end:
>>       return 0;
>>   }
>>
>> +int ff_hw_base_rc_mode_configure(AVCodecContext *avctx, const
>HWBaseEncodeRCMode *rc_mode,
>> +                                 int default_quality, HWBaseEncodeRCConfigure *rc_conf)
>> +{
>> +    HWBaseEncodeContext *ctx = avctx->priv_data;
>> +
>> +    if (!rc_mode || !rc_conf)
>> +        return -1;
>> +
>> +    if (rc_mode->bitrate) {
>> +        if (avctx->bit_rate <= 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Bitrate must be set for %s "
>> +                   "RC mode.\n", rc_mode->name);
>> +            return AVERROR(EINVAL);
>> +        }
>> +
>> +        if (rc_mode->mode == RC_MODE_AVBR) {
>> +            // For maximum confusion AVBR is hacked into the existing API
>> +            // by overloading some of the fields with completely different
>> +            // meanings.
>
>You definitely don't want this absurd wart from libva to leak into the common
>API parts.  Make new fields which have the desired meaning in the common
>parts and let the VAAPI code deal with this stupidity.

Agreed.

>
>> +
>> +            // Target percentage does not apply in AVBR mode.
>> +            rc_conf->rc_bits_per_second = avctx->bit_rate;
>> +
>> +            // Accuracy tolerance range for meeting the specified target
>> +            // bitrate.  It's very unclear how this is actually intended
>> +            // to work - since we do want to get the specified bitrate,
>> +            // set the accuracy to 100% for now.
>> +            rc_conf->rc_target_percentage = 100;
>> +
>> +            // Convergence period in frames.  The GOP size reflects the
>> +            // user's intended block size for cutting, so reusing that
>> +            // as the convergence period seems a reasonable default.
>> +            rc_conf->rc_window_size = avctx->gop_size > 0 ? avctx->gop_size :
>60;
>> +
>> +        } else if (rc_mode->maxrate) {
>> +            if (avctx->rc_max_rate > 0) {
>> +                if (avctx->rc_max_rate < avctx->bit_rate) {
>> +                    av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: "
>> +                           "bitrate (%"PRId64") must not be greater than "
>> +                           "maxrate (%"PRId64").\n", avctx->bit_rate,
>> +                           avctx->rc_max_rate);
>> +                    return AVERROR(EINVAL);
>> +                }
>> +                rc_conf->rc_bits_per_second   = avctx->rc_max_rate;
>> +                rc_conf->rc_target_percentage = (avctx->bit_rate * 100) /
>> +                                                 avctx->rc_max_rate;
>> +            } else {
>> +                // We only have a target bitrate, but this mode requires
>> +                // that a maximum rate be supplied as well.  Since the
>> +                // user does not want this to be a constraint, arbitrarily
>> +                // pick a maximum rate of double the target rate.
>> +                rc_conf->rc_bits_per_second   = 2 * avctx->bit_rate;
>> +                rc_conf->rc_target_percentage = 50;
>> +            }
>> +        } else {
>> +            if (avctx->rc_max_rate > avctx->bit_rate) {
>> +                av_log(avctx, AV_LOG_WARNING, "Max bitrate is ignored "
>> +                       "in %s RC mode.\n", rc_mode->name);
>> +            }
>> +            rc_conf->rc_bits_per_second   = avctx->bit_rate;
>> +            rc_conf->rc_target_percentage = 100;
>> +        }
>> +    } else {
>> +        rc_conf->rc_bits_per_second   = 0;
>> +        rc_conf->rc_target_percentage = 100;
>> +    }
>> +
>> +    if (rc_mode->quality) {
>> +        if (ctx->explicit_qp) {
>> +            rc_conf->rc_quality = ctx->explicit_qp;
>> +        } else if (avctx->global_quality > 0) {
>> +            rc_conf->rc_quality = avctx->global_quality;
>> +        } else {
>> +            rc_conf->rc_quality = default_quality;
>> +            av_log(avctx, AV_LOG_WARNING, "No quality level set; "
>> +                   "using default (%d).\n", rc_conf->rc_quality);
>> +        }
>> +    } else {
>> +        rc_conf->rc_quality = 0;
>> +    }
>> +
>> +    if (rc_mode->hrd) {
>> +        if (avctx->rc_buffer_size)
>> +            rc_conf->hrd_buffer_size = avctx->rc_buffer_size;
>> +        else if (avctx->rc_max_rate > 0)
>> +            rc_conf->hrd_buffer_size = avctx->rc_max_rate;
>> +        else
>> +            rc_conf->hrd_buffer_size = avctx->bit_rate;
>> +        if (avctx->rc_initial_buffer_occupancy) {
>> +            if (avctx->rc_initial_buffer_occupancy > rc_conf->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, rc_conf->hrd_buffer_size);
>> +                return AVERROR(EINVAL);
>> +            }
>> +            rc_conf->hrd_initial_buffer_fullness = avctx-
>>rc_initial_buffer_occupancy;
>> +        } else {
>> +            rc_conf->hrd_initial_buffer_fullness = rc_conf->hrd_buffer_size * 3 /
>4;
>> +        }
>> +
>> +        rc_conf->rc_window_size = (rc_conf->hrd_buffer_size * 1000) / rc_conf-
>>rc_bits_per_second;
>> +    } else {
>> +        if (avctx->rc_buffer_size || avctx->rc_initial_buffer_occupancy) {
>> +            av_log(avctx, AV_LOG_WARNING, "Buffering settings are ignored "
>> +                   "in %s RC mode.\n", rc_mode->name);
>> +        }
>> +
>> +        rc_conf->hrd_buffer_size             = 0;
>> +        rc_conf->hrd_initial_buffer_fullness = 0;
>> +
>> +        if (rc_mode->mode != RC_MODE_AVBR) {
>> +            // Already set (with completely different meaning) for AVBR.
>> +            rc_conf->rc_window_size = 1000;
>> +        }
>> +    }
>> +
>> +    if (rc_conf->rc_bits_per_second          > UINT32_MAX ||
>> +        rc_conf->hrd_buffer_size             > UINT32_MAX ||
>> +        rc_conf->hrd_initial_buffer_fullness > UINT32_MAX) {
>> +        av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or "
>> +               "greater are not supported by hardware.\n");
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s.\n", rc_mode->name);
>> +
>> +    if (rc_mode->quality)
>> +        av_log(avctx, AV_LOG_VERBOSE, "RC quality: %d.\n", rc_conf-
>>rc_quality);
>> +
>> +    if (rc_mode->hrd) {
>> +        av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, "
>> +               "initial fullness %"PRId64" bits.\n",
>> +               rc_conf->hrd_buffer_size, rc_conf->hrd_initial_buffer_fullness);
>> +    }
>> +
>> +    if (avctx->framerate.num > 0 && avctx->framerate.den > 0)
>> +        av_reduce(&rc_conf->fr_num, &rc_conf->fr_den,
>> +                  avctx->framerate.num, avctx->framerate.den, 65535);
>> +    else
>> +        av_reduce(&rc_conf->fr_num, &rc_conf->fr_den,
>> +                  avctx->time_base.den, avctx->time_base.num, 65535);
>> +
>> +    av_log(avctx, AV_LOG_VERBOSE, "RC framerate: %d/%d (%.2f fps).\n",
>> +           rc_conf->fr_num, rc_conf->fr_den, (double)rc_conf->fr_num /
>rc_conf->fr_den);
>> +
>> +    ctx->rc_quality  = rc_conf->rc_quality;
>> +
>> +    return 0;
>> +}
>> +
>>   int ff_hw_base_encode_init(AVCodecContext *avctx)
>>   {
>>       HWBaseEncodeContext *ctx = avctx->priv_data;
>> diff --git a/libavcodec/hw_base_encode.h b/libavcodec/hw_base_encode.h
>> index b836b22e6b..4072b514d3 100644
>> --- a/libavcodec/hw_base_encode.h
>> +++ b/libavcodec/hw_base_encode.h
>> @@ -72,6 +72,37 @@ enum {
>>       RC_MODE_MAX = RC_MODE_AVBR,
>>   };
>>
>> +typedef struct HWBaseEncodeRCMode {
>> +    // Mode from above enum (RC_MODE_*).
>> +    int mode;
>> +    // Name.
>> +    const char *name;
>> +    // Uses bitrate parameters.
>> +    int bitrate;
>> +    // Supports maxrate distinct from bitrate.
>> +    int maxrate;
>> +    // Uses quality value.
>> +    int quality;
>> +    // Supports HRD/VBV parameters.
>> +    int hrd;
>> +} HWBaseEncodeRCMode;
>> +
>> +typedef struct HWBaseEncodeRCConfigure
>> +{
>> +    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;
>> +    int fr_den;
>> +} HWBaseEncodeRCConfigure;
>
>The set of fields here needs more thought to match up the common parts of
>the APIs.
>
>Just have target rate and maxrate and let VAAPI deal with the percentage stuff
>maybe?  Not sure what to do with window_size which isn't present at all in
>D3D12.  The convergence/accuracy stuff for VAAPI AVBR is also unclear.
>

Could you please explain more about your thoughts on why the percentage stuff should not be in the common part? I think D3D12 can share the same code in terms of percentage stuff and hrd stuff. I can let VAAPI do the AVBR stuff and remove window_size from common part and keep everything else as-is.

>(Do you know why QVBR is missing the VBV parameters in D3D12?  On the face
>of it that doesn't make any sense, but maybe I'm missing something.)
>

It is presented in QVBR1 structure(defined in DirectX-header but not yet in Windows SDK).
I can add this support afterwards maybe along with AV1 implementation.

>> +
>> +
>>   typedef struct HWBaseEncodePicture {
>>       struct HWBaseEncodePicture *next;
>>
>> @@ -242,6 +273,9 @@ int
>ff_hw_base_encode_set_output_property(AVCodecContext *avctx,
>HWBaseEncodePic
>>
>>   int ff_hw_base_encode_receive_packet(AVCodecContext *avctx, AVPacket
>*pkt);
>>
>> +int ff_hw_base_rc_mode_configure(AVCodecContext *avctx, const
>HWBaseEncodeRCMode *rc_mode,
>> +                                 int default_quality, HWBaseEncodeRCConfigure *rc_conf);
>> +
>>   int ff_hw_base_encode_init(AVCodecContext *avctx);
>>
>>   int ff_hw_base_encode_close(AVCodecContext *avctx);
>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
>> ...
>
>Thanks,
>
>- Mark

Thanks for your review and I'll reply to your comments regarding D3D12 HEVC encoder patch later since there're indeed a lot of stuff you pointed out that I need to take care of again.

BRs,
Tong 

>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".