-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
encoder: Add CQP mode and Tuning #73
Conversation
62ed77b
to
33083ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clear and easy to review PR. My only reason for not merging it as-is and addressing issues with follow-up patches is the build breakage that is introduced in the middle - if you could at least address this point, we can always come to the other ones later (although most of them should also be trivial to fix).
|
||
let init_qp = if let RateControl::ConstantQuality(init_qp) = self.tunings.rate_control { | ||
// Limit QP to valid values | ||
init_qp.clamp(MIN_QP as u32, MAX_QP as u32) as u8 | ||
init_qp.clamp(min_qp, max_qp) as u8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: I'm wondering if we couldn't/shouldn't turn all these clamped values into their own types that implement From<u32>
and enforce that the value is clamped. Then the use of the type would ensure we haven't accidentally missed clamping a value somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like this idea and I'd love to do that. However I've decided to create a Tunings
struct codec agnostic. I'd like to avoid creating an enum with StatelessVideoEncoder<Handle, Codec>
(in future VideoEncoder<...>
) per each codec in our use cases, which would be necessary to accommodate those types in Tunings
.
This change modifies Bitrate enum so in the future it can be used for specifying non bitrate rate driven encoder operation. The enum was renamed to RateControl and the target bitrate was renamed and made an option.
This change adds constant quality encoding mode ie. the encoder shall maintain codec specific quality parameter constant disregarding bitrate. With this change the ccenc's default_qp parameter is no longer viable and removed.
This change export codec agnostic code of LowDelay to a new separate module. The codec specific code will be implemented using a delegate type in codec module. This commit includes only deduplication of AV1 LowDelay, other codecs will follow in following commits.
This change removes duplicated code from VP9's LowDelay predictor.
This change removes duplicated code from H264's LowDelay predictor. With this change LowDelay tail size greater then 1 is no longer supported. This may be added in the future, however the contribution to compression performance may not be worth it. With this commit the duplication process of LowDelay predictor is done.
This change add tuning mechanism to stateless encoder. It enables the client to change some encoding parameters of the steam dynamically without need to recreate the entire encoder instance.
This change enables tuning mechanism on AV1 encoder. It is fairly limited as the AV1 encoder works only with CQ mode.
This change enables tuning mechanism on VP9 encoder.
This change enables tuning mechanism on h264 encoder.
This change submits additional parameters from VAAPI backend, like min and max values of QP parameters using VAEncMiscParameterRateControl.
This change modifies BackendReqeust to include frame Tunings. This enables passing min and max Q values to the backend. Currently those will not affect the resulting bitstream as the CQ mode is only support. On top of that the change includes clamping the requested constant Q index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Gnurou a lot for a great review once again! 😄 Hopefully all comments were answered 😅
|
||
let init_qp = if let RateControl::ConstantQuality(init_qp) = self.tunings.rate_control { | ||
// Limit QP to valid values | ||
init_qp.clamp(MIN_QP as u32, MAX_QP as u32) as u8 | ||
init_qp.clamp(min_qp, max_qp) as u8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like this idea and I'd love to do that. However I've decided to create a Tunings
struct codec agnostic. I'd like to avoid creating an enum with StatelessVideoEncoder<Handle, Codec>
(in future VideoEncoder<...>
) per each codec in our use cases, which would be necessary to accommodate those types in Tunings
.
This change fixes propagation of level_idc in H.264 encoder. Prior to that change the default Level value was used and in some cases could cause decoder to fail to decoder the stream correctly.
This change submits additional parameters from VAAPI backend using VAEncMiscParameterRateControl. With this change VP9 VAAPI encoder responds to tuning request
Merged after fixing a build failure in the last 3 CLs - thanks! |
No description provided.