Use software bitrate control for D3D12 H26x video encoder [chromium/src : main]
Pohsiang Hsu (Gerrit)
Pohsiang Hsu added 3 comments![]( "Open in Gerrit")
pic_params_.PictureOrderCountNumber = -1;Just curious, this seems to suggest we insert a keyframe when frame_rate exceeds frame_rate_max. Is that the case?
software_rate_controller_->temporal_layers(0).SetBufferParameters(Just wondering, I see that if framerate changed and is higher than frame_rate_max, then we update gop_max_duration and frame_rate. But what about when framerate changes to be lower than frame_rate_max, we don't need to update gop_max_duration and frame_rate? most like this is by design, but I'd just wanted to ask just in case.
(config.gop_length.value() + config.framerate - 1) / config.framerate);wondering if there is any check for config.framerate > 0 in the caller of InitializeVideoEncoder? if not, perhaps would be good to add a check for safety (framerate > 0)?
Related details
- Eugene Zemtsov
- Zhibo Wang
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 3 comments![]( "Open in Gerrit")
Just curious, this seems to suggest we insert a keyframe when frame_rate exceeds frame_rate_max. Is that the case?
It was. Thanks to your comment, I think this is unnecessary. It is removed now.
software_rate_controller_->temporal_layers(0).SetBufferParameters(Just wondering, I see that if framerate changed and is higher than frame_rate_max, then we update gop_max_duration and frame_rate. But what about when framerate changes to be lower than frame_rate_max, we don't need to update gop_max_duration and frame_rate? most like this is by design, but I'd just wanted to ask just in case.
It was my design, when it don't exceed the maximum we won't use up the bitrate budget, so it is ok we don't reset it. But I feel it kind of wierd now. I have updated the logic to as long as the frame rate changes, the controller is reset, so it can provide precise estimation according to the new frame rate.
(config.gop_length.value() + config.framerate - 1) / config.framerate);wondering if there is any check for config.framerate > 0 in the caller of InitializeVideoEncoder? if not, perhaps would be good to add a check for safety (framerate > 0)?
It will be by default 30 or what we explicitly set. Adding a check will be safer. Done.
Related details
- Eugene Zemtsov
- Pohsiang Hsu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pohsiang Hsu (Gerrit)
Pohsiang Hsu added 1 comment![]( "Open in Gerrit")
- Eugene Zemtsov
- Zhibo Wang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhibo Wang (Gerrit)
Zhibo Wang added 1 comment![]( "Open in Gerrit")
- Eugene Zemtsov
- Rafael Cintron
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 1 comment![]( "Open in Gerrit")
base::TimeDelta rate_controller_timestamp_;Can you please write a comment detailing what this "timestamp" is used for and how it is calculated? To me, it looks more like a time delta than a "stamp" like base::TimeTicks is.
Related details
- Eugene Zemtsov
- Zhibo Wang
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 1 comment![]( "Open in Gerrit")
Can you please write a comment detailing what this "timestamp" is used for and how it is calculated? To me, it looks more like a time delta than a "stamp" like base::TimeTicks is.
Done. This refers to the timestamp of a frame in the video, which start with 0.
Related details
- Eugene Zemtsov
- Rafael Cintron
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 1 comment![]( "Open in Gerrit")
base::TimeDelta rate_controller_timestamp_;Zhibo WangCan you please write a comment detailing what this "timestamp" is used for and how it is calculated? To me, it looks more like a time delta than a "stamp" like base::TimeTicks is.
Done. This refers to the timestamp of a frame in the video, which start with 0.
To me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure. From the looks of the code, we increment the time delta by the framerate, but only in `D3D12VideoEncodeH265Delegate::ReadbackBitstream`. Is there significance of doing so here and not elsewhere? Is ReadbackBitsstream meant to be a heartbeat of encoding?
Related details
- Eugene Zemtsov
- Zhibo Wang
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 1 comment![]( "Open in Gerrit")
base::TimeDelta rate_controller_timestamp_;Zhibo WangCan you please write a comment detailing what this "timestamp" is used for and how it is calculated? To me, it looks more like a time delta than a "stamp" like base::TimeTicks is.
Rafael CintronDone. This refers to the timestamp of a frame in the video, which start with 0.
To me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure. From the looks of the code, we increment the time delta by the framerate, but only in `D3D12VideoEncodeH265Delegate::ReadbackBitstream`. Is there significance of doing so here and not elsewhere? Is ReadbackBitsstream meant to be a heartbeat of encoding?
To make it simple, all we are doing in this CL is to use a software (previously it is done by hardware) rate controller in our encoding procedure, which takes the encoded stream byte size and the timestamp (the one we are talking about) of the current frame as input, and produces the QP value for next frame.
To me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure.
It makes sense in general coding area, but here we are in media domain, where `timestamp` may be like a term that has special meaning. It stands for the time offset since the start of the video that a frame should be presented at. So the `TimeDelta` means delta to the start of the video, which makes sense to me, while `TimeTicks` is real world time, which don't fit our definition.
Also, this variable is for `H264RateController`, who accepts a `base::TimeDelta` object which is named as timestamp.
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/h264_rate_controller.h;l=502;drc=3ddadc368c8043783e32a3b50c64158cfb8aa42a
Is ReadbackBitsstream meant to be a heartbeat of encoding?
`ReadbackBitsstream()` will be called once a frame is encoded. So it may be yes but we don't utilize the real world time when it is called, in this CL. We are updating the rate controller to let it calculate the remaining bandwidth and the proper QP value for next frame. We do the update in `ReadbackBitsstream()` is only because we had to wait until this time to know the size of encoded stream.
Related details
- Eugene Zemtsov
- Rafael Cintron
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rafael Cintron
| Code-Review | +1 |
1 comment
base::TimeDelta rate_controller_timestamp_;Zhibo WangCan you please write a comment detailing what this "timestamp" is used for and how it is calculated? To me, it looks more like a time delta than a "stamp" like base::TimeTicks is.
Rafael CintronDone. This refers to the timestamp of a frame in the video, which start with 0.
Zhibo WangTo me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure. From the looks of the code, we increment the time delta by the framerate, but only in `D3D12VideoEncodeH265Delegate::ReadbackBitstream`. Is there significance of doing so here and not elsewhere? Is ReadbackBitsstream meant to be a heartbeat of encoding?
To make it simple, all we are doing in this CL is to use a software (previously it is done by hardware) rate controller in our encoding procedure, which takes the encoded stream byte size and the timestamp (the one we are talking about) of the current frame as input, and produces the QP value for next frame.
To me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure.
It makes sense in general coding area, but here we are in media domain, where `timestamp` may be like a term that has special meaning. It stands for the time offset since the start of the video that a frame should be presented at. So the `TimeDelta` means delta to the start of the video, which makes sense to me, while `TimeTicks` is real world time, which don't fit our definition.
Also, this variable is for `H264RateController`, who accepts a `base::TimeDelta` object which is named as timestamp.
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/h264_rate_controller.h;l=502;drc=3ddadc368c8043783e32a3b50c64158cfb8aa42aIs ReadbackBitsstream meant to be a heartbeat of encoding?
`ReadbackBitsstream()` will be called once a frame is encoded. So it may be yes but we don't utilize the real world time when it is called, in this CL. We are updating the rate controller to let it calculate the remaining bandwidth and the proper QP value for next frame. We do the update in `ReadbackBitsstream()` is only because we had to wait until this time to know the size of encoded stream.
Thank you for this detailed explanation. Please include a summary of the explanation in either the comment for the member or in the place where you increment it.
Related details
- Eugene Zemtsov
- Zhibo Wang
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 1 comment![]( "Open in Gerrit")
base::TimeDelta rate_controller_timestamp_;Zhibo WangCan you please write a comment detailing what this "timestamp" is used for and how it is calculated? To me, it looks more like a time delta than a "stamp" like base::TimeTicks is.
Rafael CintronDone. This refers to the timestamp of a frame in the video, which start with 0.
Zhibo WangTo me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure. From the looks of the code, we increment the time delta by the framerate, but only in `D3D12VideoEncodeH265Delegate::ReadbackBitstream`. Is there significance of doing so here and not elsewhere? Is ReadbackBitsstream meant to be a heartbeat of encoding?
Rafael CintronTo make it simple, all we are doing in this CL is to use a software (previously it is done by hardware) rate controller in our encoding procedure, which takes the encoded stream byte size and the timestamp (the one we are talking about) of the current frame as input, and produces the QP value for next frame.
To me, a timestamp would use the base.TimeTicks data structure, not the base.TimeDelta data structure.
It makes sense in general coding area, but here we are in media domain, where `timestamp` may be like a term that has special meaning. It stands for the time offset since the start of the video that a frame should be presented at. So the `TimeDelta` means delta to the start of the video, which makes sense to me, while `TimeTicks` is real world time, which don't fit our definition.
Also, this variable is for `H264RateController`, who accepts a `base::TimeDelta` object which is named as timestamp.
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/h264_rate_controller.h;l=502;drc=3ddadc368c8043783e32a3b50c64158cfb8aa42aIs ReadbackBitsstream meant to be a heartbeat of encoding?
`ReadbackBitsstream()` will be called once a frame is encoded. So it may be yes but we don't utilize the real world time when it is called, in this CL. We are updating the rate controller to let it calculate the remaining bandwidth and the proper QP value for next frame. We do the update in `ReadbackBitsstream()` is only because we had to wait until this time to know the size of encoded stream.
Thank you for this detailed explanation. Please include a summary of the explanation in either the comment for the member or in the place where you increment it.
- Eugene Zemtsov
- Rafael Cintron
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
- Eugene Zemtsov
- Zhibo Wang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 1 comment![]( "Open in Gerrit")
layer_settings.peak_bitrate = bitrate.peak_bps();peak_bps() isn't supposed to be called with CBR. same in the h265 encoder
https://source.chromium.org/chromium/chromium/src/+/main:media/base/bitrate.cc;l=33
Related details
- Zhibo Wang
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 1 comment![]( "Open in Gerrit")
peak_bps() isn't supposed to be called with CBR. same in the h265 encoder
https://source.chromium.org/chromium/chromium/src/+/main:media/base/bitrate.cc;l=33
- Eugene Zemtsov
- Rafael Cintron
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
- Eugene Zemtsov
- Zhibo Wang
Code-Coverage
Code-Review
| 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
- Zhibo Wang
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
Use software bitrate control for D3D12 H26x video encoder
Add support of software bitrate control by calculating per-frame QP
values using H264RateController and setting CQP in D3D12.
- M media/gpu/windows/d3d12_video_encode_accelerator.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.cc
- M media/gpu/windows/d3d12_video_encode_h264_delegate.h
- M media/gpu/windows/d3d12_video_encode_h264_delegate_unittest.cc
- M media/gpu/windows/d3d12_video_encode_h265_delegate.cc
- M media/gpu/windows/d3d12_video_encode_h265_delegate.h
- M media/gpu/windows/d3d12_video_encode_h265_delegate_unittest.cc
Code-Review: +1 by Rafael Cintron, +1 by Eugene Zemtsov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |