[Camera] Migrate WebRTC Provider Cluster to Server Cluster Interface #42067
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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the WebRTC Provider Cluster to use the new Server Cluster Interface, which is a good improvement for consistency and maintainability. The changes are mostly correct and align with the new cluster implementation pattern. However, I've identified a critical bug in the HandleSolicitOffer implementation where the stream selection logic was broken during the refactoring. Please see the detailed comment.
src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.cpp
Outdated
Show resolved
Hide resolved
Copilot finished reviewing on behalf of
yufengwangca
November 20, 2025 18:08
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the WebRTC Transport Provider cluster from the legacy AttributeAccessInterface/CommandHandlerInterface pattern to the modern ServerClusterInterface using DefaultServerCluster as a base. This migration aligns with the SDK's effort to standardize cluster implementations and improve testability through a cleaner separation of concerns.
Key Changes:
- Replaced manual registration with automatic registration through CodegenDataModelProvider registry
- Changed from unique_ptr ownership to LazyRegisteredServerCluster for explicit lifecycle management
- Removed the WebRTCTransportProviderController abstraction layer, simplifying the architecture
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.h | Migrated class from AttributeAccessInterface/CommandHandlerInterface to DefaultServerCluster; updated method signatures and constructor parameter order |
| src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.cpp | Refactored ReadAttribute and InvokeCommand to use new ServerClusterInterface pattern; updated attribute change notifications to use NotifyAttributeChanged |
| src/app/clusters/webrtc-transport-provider-server/BUILD.gn | Updated build file to create a proper source_set with explicit dependencies instead of an empty group |
| src/app/clusters/webrtc-transport-provider-server/app_config_dependent_sources.gni | Cleared app_config_dependent_sources since sources are now handled directly in BUILD.gn |
| src/app/clusters/webrtc-transport-provider-server/app_config_dependent_sources.cmake | Updated to reference renamed files (webrtc-transport-provider-cluster instead of webrtc-transport-provider-server) |
| scripts/tools/check_includes_config.py | Updated file path reference for include checking |
| examples/camera-app/camera-common/src/camera-app.cpp | Changed from unique_ptr instantiation with Init/Shutdown to LazyRegisteredServerCluster with Create/Destroy and explicit registry registration |
| examples/camera-app/camera-common/include/camera-app.h | Changed member from unique_ptr to LazyRegisteredServerCluster for better lifecycle management |
| examples/camera-app/camera-common/include/webrtc-provider-controller/webrtc-provider-controller.h | Deleted file - removed controller abstraction layer |
| examples/camera-app/camera-common/include/camera-device-interface.h | Replaced GetWebRTCProviderController with SetWebRTCTransportProvider for direct server instance setting |
| examples/camera-app/camera-common/BUILD.gn | Removed reference to deleted controller header and added dependency on webrtc-transport-provider-server |
| examples/camera-app/linux/include/clusters/webrtc-provider/webrtc-provider-manager.h | Removed WebRTCTransportProviderController inheritance; changed to raw pointer for server instance |
| examples/camera-app/linux/include/camera-device.h | Updated interface method signature for SetWebRTCTransportProvider |
| examples/camera-app/linux/src/camera-device.cpp | Updated implementation to call SetWebRTCTransportProvider instead of GetWebRTCProviderController |
| examples/camera-app/linux/src/clusters/webrtc-provider/webrtc-provider-manager.cpp | Updated method signature to accept raw pointer instead of unique_ptr |
Comments suppressed due to low confidence (2)
src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.cpp:661
- The code is using
req.videoStreamIDandreq.audioStreamIDdirectly instead of the local variablesvideoStreamIDandaudioStreamIDthat were declared at lines 503-504. These local variables are passed by reference tomDelegate.ValidateStreamUsage()at line 641, which may modify them. The delegate should populate these with stream IDs when they are null. Using the original request values instead of the potentially modified local variables will cause the wrong values to be passed to the delegate'sHandleSolicitOffermethod.
Change lines 660-661 to use the local variables:
args.videoStreamId = videoStreamID;
args.audioStreamId = audioStreamID;src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.h:361
- The documentation comment says "with the specified delegate and endpoint" but the actual parameter order is
endpointIdfirst, thendelegate. Update the comment to reflect the correct parameter order:
/**
* @brief
* Constructs the WebRTCTransportProviderServer with the specified endpoint and delegate.
*
* @param[in] endpointId The Endpoint where the WebRTC Transport Provider cluster is published.
* @param[in] delegate A reference to an implementation of the Delegate interface. Must remain
* valid for the lifetime of this object.
*/
|
PR #42067: Size comparison from ee2ab5e to 74e2761 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
yufengwangca
force-pushed
the
pr/webrtcp/code
branch
from
74e2761 to
498782e
Compare
|
PR #42067: Size comparison from d4bc686 to 498782e Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
|
Contributor
|
Would you mind re-running CI before submitting this? #41955 may result in your PR breaking CI |
andy31415
reviewed
Nov 26, 2025
src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Nov 27, 2025
src/app/clusters/webrtc-transport-provider-server/WebRTCTransportProviderCluster.cpp
Show resolved
Hide resolved
yufengwangca
force-pushed
the
pr/webrtcp/code
branch
from
498782e to
b28c922
Compare
Copilot finished reviewing on behalf of
yufengwangca
November 27, 2025 16:39
|
PR #42067: Size comparison from 6fd991a to b28c922 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
reviewed
Nov 27, 2025
src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.cpp
Outdated
Show resolved
Hide resolved
…port-provider-cluster.cpp Co-authored-by: Andrei Litvin <[email protected]>
|
PR #42067: Size comparison from 6fd991a to b950736 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
|
PR #42067: Size comparison from 6fd991a to 5d53525 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.h:399
- [nitpick] The member variable ordering places
mDelegatebeforemCurrentSessions, but the private helper methods and command handlers are listed after. For better code organization and consistency with other cluster implementations (like PushAvStreamTransportCluster), consider placing all member variables together at the end of the private section, after all method declarations.
|
PR #42067: Size comparison from 6fd991a to 2adc8d3 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
reviewed
Dec 2, 2025
Contributor
andy31415
left a comment
andy31415
left a comment
There was a problem hiding this comment.
@yufengwangca code driven cluster conversions should start having unit tests.
Given that this is an existing cluster conversion, we are not asking for full coverage, however please add at least one test to prove that testing is possible. This is usually something like a test that attribute listing works or read some simple attribute (e.g. read revision and check that that works).
|
PR #42067: Size comparison from 94d3bbb to 0295002 Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
reviewed
Dec 2, 2025
src/app/clusters/webrtc-transport-provider-server/tests/TestWebRTCTransportProviderCluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Dec 2, 2025
src/app/clusters/webrtc-transport-provider-server/tests/TestWebRTCTransportProviderCluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Dec 2, 2025
src/app/clusters/webrtc-transport-provider-server/tests/TestWebRTCTransportProviderCluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Dec 2, 2025
src/app/clusters/webrtc-transport-provider-server/tests/TestWebRTCTransportProviderCluster.cpp
Outdated
Show resolved
Hide resolved
…bRTCTransportProviderCluster.cpp Co-authored-by: Andrei Litvin <[email protected]>
src/app/clusters/webrtc-transport-provider-server/tests/TestWebRTCTransportProviderCluster.cpp
Show resolved
Hide resolved
|
PR #42067: Size comparison from 94d3bbb to 7eec9e2 Full report (30 builds for bl602, bl702, bl702l, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #42067: Size comparison from 94d3bbb to 595e844 Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
approved these changes
Dec 4, 2025
andy31415
added
the
sdk-maintainer-approved
marybadalyan
pushed a commit
to marybadalyan/connectedhomeip
that referenced
this pull request
Dec 5, 2025
…roject-chip#42067) * [Camera] Migrate WebRTC Provider Cluster to Server Cluster Interface * Update src/app/clusters/webrtc-transport-provider-server/webrtc-transport-provider-cluster.cpp Co-authored-by: Andrei Litvin <[email protected]> * Address review comments * Remove un-used variables * Name things UpperCamelCaseCluster for code driven clusters * Update src/app/clusters/webrtc-transport-provider-server/tests/TestWebRTCTransportProviderCluster.cpp Co-authored-by: Andrei Litvin <[email protected]> * Address review comments --------- Co-authored-by: Andrei Litvin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Reland #40432
Related issues
N/A
Testing
Validated by CI