Feat svc simulcast #266
Feat svc simulcast #266
Conversation
WalkthroughWalkthroughThe updates primarily focus on refining syntax, enhancing logging, and expanding media handling capabilities across various files in the project. Key changes include the introduction of new structures for media packet selection and bitrate control, adjustments in function syntax, and improved metadata management within media packets. These refinements aim to enhance performance and clarity in media processing modules. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (6)
Files selected for processing (28)
Files skipped from review due to trivial changes (2)
Additional Context UsedGitHub Check Runs (2)
Additional comments not posted (61)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
giangndm
force-pushed
the
feat-svc-simulcast
branch
from
5bad59e to
d6e35b1
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Out of diff range and nitpick comments (5)
packages/media_core/src/cluster/room/channel_pub.rs (2)
81-81: Ensure that the logging statement provides all necessary information for debugging.Consider adding more context to the log message, such as the media type or any other relevant metadata.
Line range hint
102-119: Consider adding assertions to verify the correctness of the media packet handling in the test cases.Would you like me to help by adding more comprehensive assertions to these tests?
packages/media_core/src/endpoint/internal/local_track.rs (1)
22-23: Consider adding a brief comment explaining the purpose ofpacket_selector::PacketSelector.bin/public/whip/whip.js (2)
1-1: Consider removing the commented-out import if it's no longer needed.
176-177: The methodtrickleis scheduled to run immediately after setting the local description. Consider adding a comment explaining why this is necessary, as it might not be immediately clear to other developers.
| /// Mark the input as dropped. | ||
| pub fn drop_value(&mut self, input: u64) { | ||
| log::trace!("drop value {input}"); | ||
| assert!(input < MAX, "{} should < MAX {}", input, MAX); |
There was a problem hiding this comment.
Consider handling sequence number assertions more gracefully.
Using assert! to enforce sequence number constraints (input < MAX) might cause the program to panic, which is not ideal in production environments. Consider replacing assertions with error handling to gracefully manage unexpected values.
| /// Add more offset to base. | ||
| pub fn offset(&mut self, offset: u64) { | ||
| if self.reinit { | ||
| self.reinit_offset = Some(self.reinit_offset.unwrap_or(0) + offset); |
There was a problem hiding this comment.
Potential risk in using unwrap_or without validation.
The use of unwrap_or(0) in the offset method assumes that reinit_offset is always valid when reinit is true. This could lead to incorrect behavior if reinit_offset is None unexpectedly. Consider adding checks or default handling before this operation.
| selector: PacketSelector::new(kind), | ||
| timer: TimePivot::build(), |
There was a problem hiding this comment.
The constructor correctly initializes the PacketSelector and TimePivot. However, these lines are not covered by tests.
Please ensure that unit tests cover the initialization logic in the constructor to verify that the components are set up correctly.
| fn on_cluster_event(&mut self, now: Instant, event: ClusterLocalTrackEvent) -> Option<Output> { | ||
| match event { | ||
| ClusterLocalTrackEvent::Started => todo!(), | ||
| ClusterLocalTrackEvent::SourceChanged => { | ||
| let room = self.room.as_ref()?; | ||
| log::info!("[EndpointLocalTrack] source changed => request key-frame and reset seq, ts rewrite"); | ||
| Some(Output::Cluster(*room, ClusterLocalTrackControl::RequestKeyFrame)) | ||
| } | ||
| ClusterLocalTrackEvent::Media(pkt) => { | ||
| log::trace!("[EndpointLocalTrack] on media payload {} seq {}", pkt.pt, pkt.seq); | ||
| ClusterLocalTrackEvent::Media(channel, mut pkt) => { | ||
| log::trace!("[EndpointLocalTrack] on media payload {:?} seq {}", pkt.meta, pkt.seq); | ||
| let now_ms = self.timer.timestamp_ms(now); | ||
| self.selector.select(self.timer.timestamp_ms(now), channel, &mut pkt)?; | ||
| self.pop_selector(now_ms); |
There was a problem hiding this comment.
Handling of ClusterLocalTrackEvent::Media is crucial for media packet processing. However, this block is not covered by tests.
It's important to add tests for this event handling to ensure that media packets are processed correctly, especially since this involves timestamp adjustments and packet selection which are critical operations.
| self.queue.push_back(Output::Cluster(*room, ClusterLocalTrackControl::Subscribe(peer, track))); | ||
| self.selector.reset(); |
There was a problem hiding this comment.
Resetting the selector and handling subscription logic in the RPC request is a good separation of concerns. However, these lines are not covered by tests.
Consider adding unit tests for these operations to ensure that the state transitions and selector resets are handled correctly.
| } | ||
| } | ||
|
|
||
| pub fn rewrite_rtp(payload: &mut [u8], sim: &H264Sim) {} |
There was a problem hiding this comment.
The rewrite_rtp function has unused variables payload and sim, which should either be used or removed to clean up the code.
- pub fn rewrite_rtp(payload: &mut [u8], sim: &H264Sim) {}
+ pub fn rewrite_rtp(_payload: &mut [u8], _sim: &H264Sim) {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn rewrite_rtp(payload: &mut [u8], sim: &H264Sim) {} | |
| pub fn rewrite_rtp(_payload: &mut [u8], _sim: &H264Sim) {} |
| pub fn number_temporals(&self) -> u8 { | ||
| if self.0[0].is_none() { | ||
| return 0; | ||
| } | ||
|
|
||
| if self.0[1].is_none() { | ||
| return 1; | ||
| } | ||
|
|
||
| if self.0[2].is_none() { | ||
| return 2; | ||
| } | ||
|
|
||
| return 3; |
There was a problem hiding this comment.
The method number_temporals can be simplified by removing the explicit return statements.
- return 0;
+ 0
- return 1;
+ 1
- return 2;
+ 2
- return 3;
+ 3Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn number_temporals(&self) -> u8 { | |
| if self.0[0].is_none() { | |
| return 0; | |
| } | |
| if self.0[1].is_none() { | |
| return 1; | |
| } | |
| if self.0[2].is_none() { | |
| return 2; | |
| } | |
| return 3; | |
| pub fn number_temporals(&self) -> u8 { | |
| if self.0[0].is_none() { | |
| 0 | |
| } else if self.0[1].is_none() { | |
| 1 | |
| } else if self.0[2].is_none() { | |
| 2 | |
| } else { | |
| 3 | |
| } |
| return 2; | ||
| } | ||
|
|
||
| return 3; |
There was a problem hiding this comment.
The method number_layers can be simplified by removing the explicit return statement.
- return 3;
+ 3Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return 3; | |
| 3 |
| RemoteTrackEvent::Media(mut media) => { | ||
| //TODO clear self.last_layer if switched to new track | ||
| if media.layers.is_some() { | ||
| log::info!("[EndpointRemoteTrack] on layers info {:?}", media.layers); | ||
| self.last_layers = media.layers.clone(); | ||
| } | ||
|
|
||
| // We restore last_layer if key frame not contain for allow consumers fast switching | ||
| if media.meta.is_video_key() && media.layers.is_none() { | ||
| log::info!("[EndpointRemoteTrack] set layers info to key-frame {:?}", media.layers); | ||
| media.layers = self.last_layers.clone(); | ||
| } |
There was a problem hiding this comment.
Optimize the cloning of last_layers by using a reference instead of cloning the entire structure.
- self.last_layers = media.layers.clone();
+ self.last_layers = Some(&media.layers);
- media.layers = self.last_layers.clone();
+ media.layers = self.last_layers.as_ref().cloned();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| RemoteTrackEvent::Media(mut media) => { | |
| //TODO clear self.last_layer if switched to new track | |
| if media.layers.is_some() { | |
| log::info!("[EndpointRemoteTrack] on layers info {:?}", media.layers); | |
| self.last_layers = media.layers.clone(); | |
| } | |
| // We restore last_layer if key frame not contain for allow consumers fast switching | |
| if media.meta.is_video_key() && media.layers.is_none() { | |
| log::info!("[EndpointRemoteTrack] set layers info to key-frame {:?}", media.layers); | |
| media.layers = self.last_layers.clone(); | |
| } | |
| RemoteTrackEvent::Media(mut media) => { | |
| //TODO clear self.last_layer if switched to new track | |
| if media.layers.is_some() { | |
| log::info!("[EndpointRemoteTrack] on layers info {:?}", media.layers); | |
| self.last_layers = Some(&media.layers); | |
| } | |
| // We restore last_layer if key frame not contain for allow consumers fast switching | |
| if media.meta.is_video_key() && media.layers.is_none() { | |
| log::info!("[EndpointRemoteTrack] set layers info to key-frame {:?}", media.layers); | |
| media.layers = self.last_layers.as_ref().cloned(); | |
| } |
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_simulcast).flatten(); | ||
| let meta = h264::parse_rtp(&rtp.payload, profile, spatial)?; | ||
| (true, layers, meta) | ||
| } | ||
| MediaCodec::Vp8 => { | ||
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_simulcast).flatten(); | ||
| let meta = vp8::parse_rtp(&rtp.payload, spatial)?; | ||
| (true, layers, meta) | ||
| } | ||
| MediaCodec::Vp9(profile) => { | ||
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_svc).flatten(); |
There was a problem hiding this comment.
Simplify the calls to map and flatten by using and_then instead.
- let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_simulcast).flatten();
+ let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().and_then(extract_simulcast);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_simulcast).flatten(); | |
| let meta = h264::parse_rtp(&rtp.payload, profile, spatial)?; | |
| (true, layers, meta) | |
| } | |
| MediaCodec::Vp8 => { | |
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_simulcast).flatten(); | |
| let meta = vp8::parse_rtp(&rtp.payload, spatial)?; | |
| (true, layers, meta) | |
| } | |
| MediaCodec::Vp9(profile) => { | |
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_svc).flatten(); | |
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().and_then(extract_simulcast); | |
| let meta = h264::parse_rtp(&rtp.payload, profile, spatial)?; | |
| (true, layers, meta) | |
| } | |
| MediaCodec::Vp8 => { | |
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().and_then(extract_simulcast); | |
| let meta = vp8::parse_rtp(&rtp.payload, spatial)?; | |
| (true, layers, meta) | |
| } | |
| MediaCodec::Vp9(profile) => { | |
| let layers = rtp.header.ext_vals.user_values.get::<VideoLayersAllocation>().map(extract_svc).flatten(); |
* WIP: working with seq, ts rewrite. * WIP: working with single video stream * WIP: allow working with vp8 simulcast * WIP: allow re-enable vp8 egress with vp8 short-term memory * test: add vp8 sim test, packet selector test * feat: h264 simulcast * feat: vp9 svc
Pull Request
Description
This PR implement SVC and Simulcast suport
Related Issue
If this pull request is related to any issue, please mention it here.
Checklist
Screenshots
If applicable, add screenshots to help explain the changes made.
Additional Notes
Add any additional notes or context about the pull request here.