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

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