-
Notifications
You must be signed in to change notification settings - Fork 104
core: frontend: VideoStreamCreationDialog: use even default UDP ports #3375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Odd ports can be assumed as allocated to audio streams - May improve stream detection robustness with control stations like QGroundControl
Reviewer's GuideThis PR modifies the default UDP port assignment logic in the VideoStreamCreationDialog to ensure even ports by doubling the index offset, thereby enhancing compatibility with control stations like QGroundControl. Flow Diagram for Default UDP Port Assignmentflowchart TD
subgraph "VideoStreamCreationDialog: updateStreamEndpoint()"
A[Start] --> B{Stream Type?};
B -- "StreamType.UDP" --> C{Endpoint set?};
C -- "No" --> D["Set endpoint with port: 5600 + 2 * index"];
D --> E[End];
C -- "Yes" --> E;
B -- "StreamType.UDP265" --> F{Endpoint set?};
F -- "No" --> G["Set endpoint with port: 5600 + 2 * index"];
G --> E;
F -- "Yes" --> E;
B -- "Other" --> H[...];
H --> E;
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ES-Alexander - I've reviewed your changes - here's some feedback:
- Extract the base port (5600) and the even-offset multiplier (2) into named constants or a helper to avoid magic numbers and clarify the port calculation logic.
- Add a boundary check to ensure
5600 + 2 * index
doesn’t exceed the valid UDP port range (1–65535) or collide with other reserved ports. - Consider refactoring the duplicated UDP and UDP265 endpoint setup into a shared function to DRY up the code and simplify maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the base port (5600) and the even-offset multiplier (2) into named constants or a helper to avoid magic numbers and clarify the port calculation logic.
- Add a boundary check to ensure `5600 + 2 * index` doesn’t exceed the valid UDP port range (1–65535) or collide with other reserved ports.
- Consider refactoring the duplicated UDP and UDP265 endpoint setup into a shared function to DRY up the code and simplify maintenance.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Should we do the same on the MCM's default UDP profile? |
@joaoantoniocardoso I don't expect it would hurt 🤷 |
I don't see a reason to have the audio and the video in different port, the idea is to merge both together in our end. The concept of splitting is from the old companion, that is not an good example. |
After discussing with @joaoantoniocardoso we believe that will be much easier to receive and send the audio using different channels. |
Summary by Sourcery
Enhancements: