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

[Mark Thompson]

On 22/02/2024 10:10, Wu, Tong1 wrote:
>> On 18/02/2024 08:45, tong1.wu-at-intel.com at ffmpeg.org wrote:
>>> From: Tong Wu <tong1.wu at 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 at 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