[WEBRTCP] Add Streamusage validation for WebRTCP #39973
Open
+200
−52
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}
There was a problem hiding this comment.
Code Review
This pull request adds validation for stream usage in WebRTCP command handling, ensuring that the requested usage is supported and associated with an allocated stream. The changes span across the camera app, server logic, and test scripts.
My review identified a few issues:
- A critical logic bug in
ValidateStreamUsagethat could lead to incorrect validation failures. - A high-severity issue where a snapshot size check was commented out, posing a potential resource exhaustion risk.
- A couple of medium-severity issues related to code efficiency and use of magic numbers in tests.
I've provided specific suggestions to address these points. Overall, the changes are well-aligned with the PR's objective, and the new test cases are a great addition.
examples/camera-app/linux/src/clusters/camera-avstream-mgmt/camera-av-stream-manager.cpp
Outdated
Show resolved
Hide resolved
examples/camera-app/linux/src/clusters/camera-avstream-mgmt/camera-av-stream-manager.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/camera-av-stream-management-server/camera-av-stream-management-server.cpp
Outdated
Show resolved
Hide resolved
|
PR #39973: Size comparison from 05171f2 to 72491eb Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| CHIP_ERROR err = mDelegate.ValidateStreamUsage(req.streamUsage, videoStreamID, audioStreamID); | ||
| if (err != CHIP_NO_ERROR) | ||
| { | ||
| ChipLogError(Zcl, "HandleSolicitOffer: Cannot provide the stream usage requested"); |
There was a problem hiding this comment.
Why return DynamicConstraintError?
If not able to meet the conditions or are unable to provide another WebRTC session:
Invoke End with Reason set to OutOfResources, and end processing with no other side effects.
There was a problem hiding this comment.
ValidateStreamUsage is covering the cases above that text that explicitly call out failure with DYNAMIC_CONSTRAINT_ERROR
- stream usage is not in stream usage priorities
- video stream is present and doesn't match allocated
- audio stream is present and doesn't match allocated
| ChipLogError(Zcl, "HandleProvideOffer: Cannot meet resource management conditions"); | ||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::ResourceExhausted); | ||
| ChipLogError(Zcl, "HandleProvideOffer: Cannot provide stream usage requested"); | ||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::DynamicConstraintError); |
There was a problem hiding this comment.
Why return DynamicConstraintError? the spec require return ResourceExhausted here
If not able to meet the [Resource Management and Stream Priorities] conditions or are unable to provide another WebRTC session:
Respond with a response status of RESOURCE_EXHAUSTED
There was a problem hiding this comment.
As per the previous comment, failure in this case is because of one of:
- stream usage is not in stream usage priorities
- video stream is present and doesn't match allocated
- audio stream is present and doesn't match allocated
All of which necessitate DYNAMIC_CONSTRAINT_ERROR
| { | ||
| ChipLogError(Zcl, "HandleSolicitOffer: Cannot provide the stream usage requested"); | ||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::DynamicConstraintError); | ||
| return; |
There was a problem hiding this comment.
We also need to figure out how to send End command in this case per spec. Add TODO is ok
There was a problem hiding this comment.
In the SolicitOffer case? There has been no actually Offer/Answer exchange in this failure setups, so no WebRTC stream has even been initiated.
| @@ -335,12 +338,22 @@ void WebRTCTransportProviderServer::HandleSolicitOffer(HandlerContext & ctx, con | |||
| } | |||
| } | |||
|
|
|||
| // Check resource management and stream priorities. If the IDs are Null the delegate will populate with | |||
There was a problem hiding this comment.
Suggested change
| // Check resource management and stream priorities. If the IDs are Null the delegate will populate with | |
| // Check resource management and stream priorities. If the IDs are null the delegate will populate with |
Comment on lines
204
to
205
| * @param[in] videoStreamId Optional identifier for the requested video stream. | ||
| * @param[in] audioStreamId Optional identifier for the requested audio stream. |
There was a problem hiding this comment.
These are now [in,out] parameters, no?
| @@ -197,16 +197,17 @@ class Delegate | |||
| * The implementation SHALL ensure: | |||
| * - The requested stream usage (streamUsage) is allowed given the current allocation of | |||
| * camera resources (e.g. CPU, memory, network bandwidth) and the prioritized stream list. | |||
| * - If the provide IDs are null, it matches against an allocated stream with the same stream usage | |||
There was a problem hiding this comment.
Suggested change
| * - If the provide IDs are null, it matches against an allocated stream with the same stream usage | |
| * - If the provided IDs are null, it matches against an allocated stream with the same stream usage |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Add validation of the provided stream usage to WebRTCP command handling.
Validation is based on the stream usage being 1) supported, and 2) associated with the allocated stream.
Updated the server, app, and Test Script WEBRTCP_2_5
Testing
Run WEBRTCP_2_* test scripts via the Python Test Runner against the updated camera app
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines