Re: [FFmpeg-devel] [PATCH v5 4/9] avcodec/vaapi_encode: extract rc parameter configuration to base layer
>-Original Message-
>From: ffmpeg-devel 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@ffmpeg.org wrote:
>>>> From: Tong Wu
>>>>
>>>> VAAPI and D3D12VA can share rate control configuration codes. Hence, it
>>>> can be moved to base layer for simplification.
>>>>
>>>> Signed-off-by: Tong Wu
>>>> ---
>>>>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;
>>
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@ffmpeg.org wrote:
From: Tong Wu
VAAPI and D3D12VA can share rate control configuration codes. Hence, it
can be moved to base layer for simplification.
Signed-off-by: Tong Wu
---
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v5 4/9] avcodec/vaapi_encode: extract rc parameter configuration to base layer
>On 18/02/2024 08:45, tong1.wu-at-intel@ffmpeg.org wrote:
>> From: Tong Wu
>>
>> VAAPI and D3D12VA can share rate control configuration codes. Hence, it
>> can be moved to base layer for simplification.
>>
>> Signed-off-by: Tong Wu
>> ---
>> 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;
>> +}
>> +
Re: [FFmpeg-devel] [PATCH v5 4/9] avcodec/vaapi_encode: extract rc parameter configuration to base layer
On 18/02/2024 08:45, tong1.wu-at-intel@ffmpeg.org wrote:
From: Tong Wu
VAAPI and D3D12VA can share rate control configuration codes. Hence, it
can be moved to base layer for simplification.
Signed-off-by: Tong Wu
---
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_initia
[FFmpeg-devel] [PATCH v5 4/9] avcodec/vaapi_encode: extract rc parameter configuration to base layer
From: Tong Wu
VAAPI and D3D12VA can share rate control configuration codes. Hence, it
can be moved to base layer for simplification.
Signed-off-by: Tong Wu
---
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.
+
+// 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 (%"
