[WebRTC] Update ICECandidates/ProvideICECandidates Commands to populate SDPMid and SDPMLineIndex #41527
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR updates the WebRTC implementation to properly populate SDPMid and SDPMLineIndex fields in ICE candidates instead of sending NULL values. The changes enable proper handling of multi-media streams (audio and video) by including the required SDP metadata when sending ICE candidates between camera and controller applications.
Key changes:
- Introduced ICECandidateInfo struct to carry candidate string, mid, and mlineIndex information
- Updated ICE candidate callbacks throughout the WebRTC stack to use structured candidate info
- Modified command building logic to populate SDPMid and SDPMLineIndex fields in ICECandidateStruct
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/camera-controller/webrtc-manager/WebRTCProviderClient.h | Updated method signature and added member variables for candidate mid storage |
| examples/camera-controller/webrtc-manager/WebRTCProviderClient.cpp | Implemented structured ICE candidate handling with SDPMid/SDPMLineIndex population |
| examples/camera-controller/webrtc-manager/WebRTCManager.h | Added ICECandidateInfo struct and updated member variable type |
| examples/camera-controller/webrtc-manager/WebRTCManager.cpp | Updated callback to extract mid information from libdatachannel candidates |
| examples/camera-app/linux/src/webrtc-transport.cpp | Updated callback signature to receive structured candidate info |
| examples/camera-app/linux/src/webrtc-libdatachannel.cpp | Modified callback to create ICECandidateInfo with extracted mid data |
| examples/camera-app/linux/src/clusters/webrtc-provider/webrtc-provider-manager.cpp | Updated ICECandidates command building to populate SDP fields |
| examples/camera-app/linux/include/webrtc-transport.h | Updated method signatures and member variable types |
| examples/camera-app/linux/include/webrtc-abstract.h | Added ICECandidateInfo struct definition and updated callback type |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where SDPMid and SDPMLineIndex were not being populated for ICE candidates by introducing an ICECandidateInfo struct to carry this information through the WebRTC stack. The changes are well-contained and logical.
However, I've identified a few issues that need attention. There is a critical lifetime issue that could lead to dangling pointers and a crash. Additionally, the fix for SDPMLineIndex is incomplete as it uses a hardcoded value, which is incorrect for multi-stream scenarios. I've also noted some code duplication and a minor optimization opportunity.
My detailed comments provide suggestions to resolve these issues. Once addressed, this will be a solid improvement.
examples/camera-app/linux/src/clusters/webrtc-provider/webrtc-provider-manager.cpp
Outdated
Show resolved
Hide resolved
examples/camera-controller/webrtc-manager/WebRTCProviderClient.cpp
Outdated
Show resolved
Hide resolved
|
PR #41527: Size comparison from 8bbe2ea to c238363 Full report (3 builds for realtek, stm32)
|
|
PR #41527: Size comparison from 8bbe2ea to 2623a2d Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41527 +/- ##
==========================================
+ Coverage 51.02% 51.03% +0.01%
==========================================
Files 1386 1385 -1
Lines 100982 100943 -39
Branches 13077 13059 -18
==========================================
- Hits 51526 51520 -6
+ Misses 49456 49423 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The SDK is sending ICE candidates with NULL values for SDPMid (0x1) and SDPMLineIndex (0x2), even though the SDP contains multiple media streams (audio and video with mid tags "audio" and "video").
The root cause is the ICECandidateStruct is being initialized with only the candidate string, but not setting the SDPMid and SDPMLineIndex fields
The camera-controller's WebRTCProviderClient also needs to be updated to populate SDPMid and SDPMLineIndex when sending ICE candidates to the provider. Let me check the current implementation and update it.
Related issues
Fixes: #41460
Testing
Launch the Camera Controller:
Open a separate terminal console.
Execute the command: ./chip-camera-controller
Commission the Camera Device:
In the controller console, initiate the commissioning process using the pairing code.
pairing onnetwork 1 20202021webrtc establish-session 1
Confirm the SDPMid and SDPMLineIndex are set in ICECandidates/ProvideICECandidates Commands