[vk-video] Make encoding coding control and rate control codec-generic.#1840
[vk-video] Make encoding coding control and rate control codec-generic.#1840jerzywilczek wants to merge 1 commit intovv-codec-genericsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes Vulkan encoding rate-control and coding-control paths codec-generic by moving codec-specific Vulkan rate control structs behind the EncodeCodec trait, instead of hard-coding H.264 logic in VulkanEncoder.
Changes:
- Add codec-generic rate control associated types + factory methods to
EncodeCodec. - Move H.264 rate control layer/info construction into
H264Codec’sEncodeCodecimpl. - Update
VulkanEncoderto build and push codec-specific rate control viaC::codec_rate_control_*and remove H.264-specific helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vk-video/src/vulkan_encoder.rs | Switch rate-control/coding-control setup to codec-generic calls; remove H.264-only helpers. |
| vk-video/src/codec/h264/encode.rs | Implement the new codec-specific rate control hooks for H.264. |
| vk-video/src/codec.rs | Extend EncodeCodec with codec-specific rate control associated types and constructors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| layer_info = layer_info | ||
| .average_bitrate(average_bitrate) | ||
| .max_bitrate(max_bitrate) | ||
| .push_next(&mut h264_layer_info[0]) | ||
| .push_next(&mut codec_layer_info[0]) | ||
| } | ||
|
|
||
| RateControl::ConstantBitrate { bitrate, .. } => { | ||
| layer_info = layer_info | ||
| .average_bitrate(bitrate) | ||
| .max_bitrate(bitrate) | ||
| .push_next(&mut h264_layer_info[0]) | ||
| .push_next(&mut codec_layer_info[0]) | ||
| } | ||
|
|
||
| RateControl::Disabled => layer_info = layer_info.push_next(&mut h264_layer_info[0]), | ||
| RateControl::Disabled => layer_info = layer_info.push_next(&mut codec_layer_info[0]), |
There was a problem hiding this comment.
rate_control_layers_for assumes the codec-specific layer-info slice is non-empty and unconditionally indexes [0]. Since EncodeCodec::codec_rate_control_layer_info returns Option<Vec<_>> with no guarantee that Some contains at least one element, this can panic at runtime (and also silently ignores additional layers if a codec provides more than one). Consider either enforcing/documenting a 1-layer contract at the trait level (e.g., return a single layer info), or iterating over all provided codec layers and generating matching VideoEncodeRateControlLayerInfoKHR entries; at minimum, use get_mut(0) and return None/error when empty.
| type CodecRateControlLayerInfo<'a>: vk::ExtendsVideoEncodeRateControlLayerInfoKHR; | ||
| type CodecRateControlInfo<'a>: vk::ExtendsVideoBeginCodingInfoKHR | ||
| + vk::ExtendsVideoCodingControlInfoKHR; | ||
| fn codec_rate_control_layer_info<'a>( | ||
| rate_control: RateControl, | ||
| ) -> Option<Vec<Self::CodecRateControlLayerInfo<'a>>>; | ||
| fn codec_rate_control_info<'a>( | ||
| layers: Option<&'a [vk::VideoEncodeRateControlLayerInfoKHR<'a>]>, | ||
| idr_period: u32, | ||
| ) -> Option<Self::CodecRateControlInfo<'a>>; |
There was a problem hiding this comment.
The new EncodeCodec::codec_rate_control_layer_info API returns Option<Vec<_>>, but callers currently assume (1) Some implies at least one element and (2) only the first element is used to build VideoEncodeRateControlLayerInfoKHR. To avoid future panics and ambiguous multi-layer semantics for codec implementers, consider tightening the trait contract (e.g., return a single layer info, or specify that the vector must be non-empty and that its length must match the temporal layer count the codec will report).
No description provided.