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

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(-)

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.

> +
> +            // 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.

(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.)

> +
> +
>   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

>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".

On 22/02/2024 10:10, Wu, Tong1 wrote:
>> 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.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.

The libavcodec API is a better match for D3D12 than VAAPI is: e.g. for D3D12 VBR you have TargetAvgBitRate = AVCodecContext.bit_rate, PeakBitRate = AVCodecContext.rc_max_rate etc. with all fields nicely matching.

If you use the VAAPI fields here then you are translating the rate into a percentage (with rounding error because it's an integer) and then back again for no reason, hence I think you should prefer the libavcodec/D3D12 fields and do the conversion for VAAPI in its own code.

>> (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.

Ah, so it was a bug and has been fixed.  That's good!

>>> +
>>> +
>>>    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.

Sure, thank you!

- Mark

>-----Original Message-----
>From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>Thompson
>Sent: Friday, February 23, 2024 3:22 AM
>To: ffmpeg-devel@ffmpeg.org
>Subject: Re: [FFmpeg-devel] [PATCH v5 4/9] avcodec/vaapi_encode: extract rc
>parameter configuration to base layer
>
>On 22/02/2024 10:10, Wu, Tong1 wrote:
>>> 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.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.
>
>The libavcodec API is a better match for D3D12 than VAAPI is: e.g. for D3D12
>VBR you have TargetAvgBitRate = AVCodecContext.bit_rate, PeakBitRate =
>AVCodecContext.rc_max_rate etc. with all fields nicely matching.
>
>If you use the VAAPI fields here then you are translating the rate into a
>percentage (with rounding error because it's an integer) and then back again
>for no reason, hence I think you should prefer the libavcodec/D3D12 fields and
>do the conversion for VAAPI in its own code.

OK, that makes sense. I think if that's the case, probably it's not necessary to have this patch to extract the code to a common function. Just let VAAPI and D3D12 do their own jobs separately. I'm going to drop this patch from this patchset.


>
>>> (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.
>
>Ah, so it was a bug and has been fixed.  That's good!
>
>>>> +
>>>> +
>>>>    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.
>
>Sure, thank you!
>
>- Mark
>_______________________________________________
>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".