[Win] Create AV1 D3D12 video encode accelerator [chromium/src : main]
Jianlin Qiu (Gerrit)
Jianlin Qiu added 4 comments![]( "Open in Gerrit")
// Sensible default values for CDEF taken from
// https://github.com/intel/libva-utils/blob/master/encode/av1encode.cI would prefer you use that from third_party/libaom/source/libaom/av1/encoder/pickcdef.c;l=750. At least we have different settings for screen content and camera content with that.
constexpr std::array<uint8_t, 8> cdef_y_pri_strength = {9, 12, 0, 6,Even for constexpr std::array, we still expect consts to be defined with kXXX format. So it should be kCdefYPriStrenth. Same for ebelow.
// SAFETY: It is safe to access the buffer since the size is 7.Instead of adding this comment, you could well get the element count of the array instead of hard-code to 7. Same for below.
cdef.CdefBits = 3;Do we really always need 8 groups of cdef strengths? libaom uses 4 groups at most.
Related details
- Eugene Zemtsov
- Jianlin Qiu
- Rafael Cintron
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jianlin Qiu (Gerrit)
Jianlin Qiu added 1 comment![]( "Open in Gerrit")
// Rfresh DPB slot 0 with current reconstructed picture.refresh
Related details
- Eugene Zemtsov
- Rafael Cintron
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jianlin Qiu (Gerrit)
Jianlin Qiu added 1 comment![]( "Open in Gerrit")
sequence_header.enable_superres = false;
sequence_header.enable_cdef = true;
sequence_header.enable_restoration = false;It is expected that when when ibc(intra-block-copy) is enbled, all these flags needs to be disabled. Please make sure cdef_bits is 0 when ibc is on.
Pohsiang Hsu (Gerrit)
Pohsiang Hsu added 4 comments![]( "Open in Gerrit")
CHECK_LT(static_cast<size_t>(tid),
VideoBitrateAllocation::kMaxTemporalLayers);might want to consider to move this check out of the loop and maybe only check it once against the largest value?
CHECK_LT(static_cast<size_t>(num_temporal_layers), VideoBitrateAllocation::kMaxTemporalLayers);
.Codec = D3D12_VIDEO_ENCODER_CODEC_HEVC,just double checking, is D3D12_VIDEO_ENCODER_CODEC_HEVC correct here? Should it be D3D12_VIDEO_ENCODER_CODEC_AV1 instead?
if (++picture_params_.PictureIndex == gop_sequence_.InterFramePeriod ||Just wondering if picture_params_.PictureIndex needs be initialized to like '-1' in the constructor? So that ++ would make the 1st picture a key frame?
Related details
- Eugene Zemtsov
- Rafael Cintron
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma added 9 comments![]( "Open in Gerrit")
// Sensible default values for CDEF taken from
// https://github.com/intel/libva-utils/blob/master/encode/av1encode.cI would prefer you use that from third_party/libaom/source/libaom/av1/encoder/pickcdef.c;l=750. At least we have different settings for screen content and camera content with that.
For cdef setting, the value from libaom doesn't show PSNR benefit on camera content, I prefer to keep align with VAAPI in the initial version.
constexpr std::array<uint8_t, 8> cdef_y_pri_strength = {9, 12, 0, 6,Even for constexpr std::array, we still expect consts to be defined with kXXX format. So it should be kCdefYPriStrenth. Same for ebelow.
Done
sequence_header.enable_superres = false;
sequence_header.enable_cdef = true;
sequence_header.enable_restoration = false;It is expected that when when ibc(intra-block-copy) is enbled, all these flags needs to be disabled. Please make sure cdef_bits is 0 when ibc is on.
The feature of intra-block-copy is not supported yet.
// SAFETY: It is safe to access the buffer since the size is 7.Instead of adding this comment, you could well get the element count of the array instead of hard-code to 7. Same for below.
Done
CHECK_LT(static_cast<size_t>(tid),
VideoBitrateAllocation::kMaxTemporalLayers);might want to consider to move this check out of the loop and maybe only check it once against the largest value?
CHECK_LT(static_cast<size_t>(num_temporal_layers), VideoBitrateAllocation::kMaxTemporalLayers);
Done
just double checking, is D3D12_VIDEO_ENCODER_CODEC_HEVC correct here? Should it be D3D12_VIDEO_ENCODER_CODEC_AV1 instead?
It should be D3D12_VIDEO_ENCODER_CODEC_AV1! Thanks!
if (++picture_params_.PictureIndex == gop_sequence_.InterFramePeriod ||Just wondering if picture_params_.PictureIndex needs be initialized to like '-1' in the constructor? So that ++ would make the 1st picture a key frame?
Done
Do we really always need 8 groups of cdef strengths? libaom uses 4 groups at most.
Done
// Rfresh DPB slot 0 with current reconstructed picture.Yingying Marefresh
Done
Related details
- Jianlin Qiu
- Pohsiang Hsu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhibo Wang (Gerrit)
Zhibo Wang added 6 comments![]( "Open in Gerrit")
#define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))`std::size`
```
_EXPORT_STD template <class _Ty, size_t _Size>
_NODISCARD constexpr size_t size(const _Ty (&)[_Size]) noexcept {
return _Size;
}
```
The result will be compile-time constant.
// SAFETY: The access is safe because |LoopFilterLevel| has 2 elements.`base::span(pic_params.LoopFilter.LoopFilterLevel);` is safe. Ditto elsewhere.
UNSAFE_BUFFERS(pic_params.ReferenceFramesReconPictureDescriptors[i])Doing so we may get rid of UNSAFE_BUFFERS checks
```
base::span descriptors = pic_params.ReferenceFramesReconPictureDescriptors;
frame_header.ref_order_hint[i] = descriptors[i].OrderHint;
```
.InterFramePeriod = config.gop_length.value_or(kDefaultGOPLength)};We have already set this value in the base delegate, if you don't need one other than 3000.
for (uint32_t i = 0; i < ARRAY_SIZE(picture_params_.ReferenceIndices); ++i) {
// SAFETY: The access is safe with the bound check above.
UNSAFE_BUFFERS(picture_params_.ReferenceIndices[i]) = 0;
}Try `std::ranges::fill(picture_params_.ReferenceIndices, 0);`
if (codec == VideoCodec::kAV1) {
rate_control.params_.cqp = {
.ConstantQP_FullIntracodedFrame = 26,
.ConstantQP_InterPredictedFrame_PrevRefOnly = 30,
.ConstantQP_InterPredictedFrame_BiDirectionalRef = 30};
rate_control.rate_control_ = {
.Mode = D3D12_VIDEO_ENCODER_RATE_CONTROL_MODE_CQP,
.ConfigParams = {.DataSize = sizeof(rate_control.params_.cqp),
.pConfiguration_CQP = &rate_control.params_.cqp},
.TargetFrameRate = {framerate, 1}};
} else {To use software BRC, my plan is to override `UpdateRateControl()` and overwrite `rate_control_` in `InitializeVideoEncoder()`, instead of changing the base delegate.
Also if possible we may split the software BRC part to another CL.
Related details
- Eugene Zemtsov
- Jianlin Qiu
- Pohsiang Hsu
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma added 5 comments![]( "Open in Gerrit")
`std::size`
```
_EXPORT_STD template <class _Ty, size_t _Size>
_NODISCARD constexpr size_t size(const _Ty (&)[_Size]) noexcept {
return _Size;
}
```The result will be compile-time constant.
Done
// SAFETY: The access is safe because |LoopFilterLevel| has 2 elements.`base::span(pic_params.LoopFilter.LoopFilterLevel);` is safe. Ditto elsewhere.
Done
UNSAFE_BUFFERS(pic_params.ReferenceFramesReconPictureDescriptors[i])Doing so we may get rid of UNSAFE_BUFFERS checks
```
base::span descriptors = pic_params.ReferenceFramesReconPictureDescriptors;
frame_header.ref_order_hint[i] = descriptors[i].OrderHint;
```
Acknowledged
.InterFramePeriod = config.gop_length.value_or(kDefaultGOPLength)};We have already set this value in the base delegate, if you don't need one other than 3000.
Done
for (uint32_t i = 0; i < ARRAY_SIZE(picture_params_.ReferenceIndices); ++i) {
// SAFETY: The access is safe with the bound check above.
UNSAFE_BUFFERS(picture_params_.ReferenceIndices[i]) = 0;
}Try `std::ranges::fill(picture_params_.ReferenceIndices, 0);`
- Zhibo Wang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma added 2 comments![]( "Open in Gerrit")
// Sensible default values for CDEF taken from
// https://github.com/intel/libva-utils/blob/master/encode/av1encode.cYingying MaI would prefer you use that from third_party/libaom/source/libaom/av1/encoder/pickcdef.c;l=750. At least we have different settings for screen content and camera content with that.
For cdef setting, the value from libaom doesn't show PSNR benefit on camera content, I prefer to keep align with VAAPI in the initial version.
Done
if (codec == VideoCodec::kAV1) {
rate_control.params_.cqp = {
.ConstantQP_FullIntracodedFrame = 26,
.ConstantQP_InterPredictedFrame_PrevRefOnly = 30,
.ConstantQP_InterPredictedFrame_BiDirectionalRef = 30};
rate_control.rate_control_ = {
.Mode = D3D12_VIDEO_ENCODER_RATE_CONTROL_MODE_CQP,
.ConfigParams = {.DataSize = sizeof(rate_control.params_.cqp),
.pConfiguration_CQP = &rate_control.params_.cqp},
.TargetFrameRate = {framerate, 1}};
} else {To use software BRC, my plan is to override `UpdateRateControl()` and overwrite `rate_control_` in `InitializeVideoEncoder()`, instead of changing the base delegate.
Also if possible we may split the software BRC part to another CL.
Done
Related details
- Zhibo Wang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted and added 3 comments![]( "Open in Gerrit")
Votes added by Rafael Cintron
| Code-Review | +1 |
3 comments
CHECK(!config.HasSpatialLayer() && !config.HasTemporalLayer())Instead of
```
CHECK(A && B);
```
do
```
CHECK(A);
CHECK(B);
```
With the second code block, if the second CHECK gets hit, you know that A is true. Makes it easier to root cause crash dumps and logs.
D3D12_VIDEO_ENCODER_AV1_PROFILE av1_profile_;I presume there will be new items added to this union over time?
Please add a comment that describes what variable in the class determines which one to use.
error << "D3D12VideoEncoder does not support input format "
<< DxgiFormatToString(support->InputFormat) << ". ";For this and other string concatenation in this CL, please use `base::StrCat` for optimal performance.
Related details
- Eugene Zemtsov
- Pohsiang Hsu
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma added 5 comments![]( "Open in Gerrit")
CHECK(!config.HasSpatialLayer() && !config.HasTemporalLayer())Instead of
```
CHECK(A && B);
```do
```
CHECK(A);
CHECK(B);
```With the second code block, if the second CHECK gets hit, you know that A is true. Makes it easier to root cause crash dumps and logs.
Acknowledged
I presume there will be new items added to this union over time?
Please add a comment that describes what variable in the class determines which one to use.
Done
I presume there will be new items added to this union over time?
Please add a comment that describes what variable in the class determines which one to use.
Done
error << "D3D12VideoEncoder does not support input format "
<< DxgiFormatToString(support->InputFormat) << ". ";For this and other string concatenation in this CL, please use `base::StrCat` for optimal performance.
Done
error << "D3D12VideoEncoder does not support input format "
<< DxgiFormatToString(support->InputFormat) << ". ";For this and other string concatenation in this CL, please use `base::StrCat` for optimal performance.
Done
Related details
- Eugene Zemtsov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Eugene Zemtsov
| Code-Review | +1 |
2 comments
CHECK_LE(1u << cdef.CdefBits, std::size(kCdefYPriStrength));no need to do it. there's a CHECK in each `std::array` access
std::copy_n(packed_frame_header.data(), packed_header_size,
bitstream_buffer.data());I'm surprised that clang doen't catch this raw pointer copy. Could you please use `span::copy_from` here?
Related details
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yingying Ma
| Commit-Queue | +1 |
2 comments
CHECK_LE(1u << cdef.CdefBits, std::size(kCdefYPriStrength));no need to do it. there's a CHECK in each `std::array` access
Done
std::copy_n(packed_frame_header.data(), packed_header_size,
bitstream_buffer.data());I'm surprised that clang doen't catch this raw pointer copy. Could you please use `span::copy_from` here?
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jianlin Qiu (Gerrit)
Jianlin Qiu added 1 comment![]( "Open in Gerrit")
D3D12_VIDEO_ENCODER_AV1_FEATURE_FLAG_REDUCED_TX_SET)) {I don't think reduced tx set is a mandatory feature. Please make this best-effort instead.
Related details
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jianlin Qiu (Gerrit)
Jianlin Qiu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jianlin Qiu
| Code-Review | +1 |
1 comment
Per offline talk, the reduce-tx-mode feature checking will be updated with follow up CL.
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
- Eugene Zemtsov
- Jianlin Qiu
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jianlin Qiu (Gerrit)
Jianlin Qiu voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Eugene Zemtsov
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Yingying Ma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma added 1 comment![]( "Open in Gerrit")
D3D12_VIDEO_ENCODER_AV1_FEATURE_FLAG_REDUCED_TX_SET)) {I don't think reduced tx set is a mandatory feature. Please make this best-effort instead.
Will add reduced tx set control in a follow up CL.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yingying Ma (Gerrit)
Yingying Ma voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Win] Create AV1 D3D12 video encode accelerator
First pass implementation of the D3D12VideoEncodeAV1Delegate.
- M media/gpu/BUILD.gn
- M media/gpu/gpu_video_encode_accelerator_factory.cc
- M media/gpu/windows/d3d12_video_encode_accelerator.cc
- M media/gpu/windows/d3d12_video_encode_accelerator.h
- A media/gpu/windows/d3d12_video_encode_av1_delegate.cc
- A media/gpu/windows/d3d12_video_encode_av1_delegate.h
- A media/gpu/windows/d3d12_video_encode_av1_delegate_unittest.cc
- M media/gpu/windows/d3d12_video_encode_delegate.cc
- M media/gpu/windows/d3d12_video_encode_delegate.h
- M media/gpu/windows/d3d12_video_encode_h264_delegate.h
- M media/gpu/windows/d3d12_video_encode_h265_delegate.h
- M media/gpu/windows/d3d12_video_encoder_wrapper.cc
- M media/gpu/windows/d3d12_video_encoder_wrapper.h
- M media/gpu/windows/d3d12_video_helpers.cc
- M media/gpu/windows/d3d12_video_helpers.h
Code-Review: +1 by Eugene Zemtsov, +1 by Rafael Cintron, +1 by Jianlin Qiu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |