RTP/SRTP port rotation functionality #807
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| port += (4 * (number - 1)) % 10000; | ||
| } | ||
| #ifdef PCAPPLAY | ||
| int port = 0; |
Check notice
Code scanning / CodeQL
Declaration hides variable Note
There was a problem hiding this comment.
Pull Request Overview
This PR introduces RTP/SRTP port rotation functionality to support audio and video stream testing with port changes during media renegotiation. The implementation separates port ranges by media type (audio/video/image) and allows dynamic port rotation via XML syntax.
Key changes:
- Splits RTP port management from unified ranges to separate audio, video, and image port ranges
- Implements port rotation functionality for both UAC and UAS scenarios
- Refactors media port assignment logic to support per-media-type port handling
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/sipp.cpp | Updates command-line options to separate audio/video/image port ranges and refactors socket binding logic |
| src/rtpstream.cpp | Implements port rotation functions for UAC/UAS audio/video streams and updates existing port management |
| src/call.cpp | Updates message creation logic to handle separate media port types and rotation syntax |
| sipp_scenarios/*.xml | Adds XML scenario examples demonstrating port rotation functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| sockaddr_update_port(&address, port_number + 1); | ||
| if (::bind(*rtcpsocket, (sockaddr *) (void *)&address, | ||
| socklen_from_addr(&address)) == 0) { | ||
| if (::bind(*rtcpsocket, (sockaddr *) (void *)&address, socklen_from_addr(&address)) != 0) { |
There was a problem hiding this comment.
The bind condition is inverted. It should be == 0 for successful binding, not != 0.
Copilot uses AI. Check for mistakes.
| next_rtp_video_port = min_rtp_video_port; | ||
| } | ||
| } else { | ||
| WARNING("rtpstream_get_uas_localport(): INVALID mediaType specified when checking upport RTP port boundary: [%s]", mediaType); |
There was a problem hiding this comment.
Typo in error message: 'upport' should be 'upper'.
| WARNING("rtpstream_get_uas_localport(): INVALID mediaType specified when checking upport RTP port boundary: [%s]", mediaType); | |
| WARNING("rtpstream_get_uas_localport(): INVALID mediaType specified when checking upper RTP port boundary: [%s]", mediaType); |
Copilot uses AI. Check for mistakes.
| rtp_header.assign(audio_packet_in.begin(), it); // Fetch leading 12 bytes | ||
|
|
||
| std::advance(it_payload_begin, sizeof(rtp_header_t)); | ||
| std::advance(it_payload_end, header_payload_size); |
There was a problem hiding this comment.
Iterator should be advanced from its current position, not from the beginning. This should be std::advance(it_payload_end, -sizeof(rtp_header_t)) to get the correct payload end position.
Copilot uses AI. Check for mistakes.
1 participant
Hello @orgads :
This change introduces SRTP/RTP port rotation functionality for AUDIO and VIDEO RTP streams which is extremely useful for testing/debugging SRTP/RTP stream behaviors when port changes occur with media renegotiation.
Since port rotation can happen on any AUDIO/VIDEO RTP stream this means that the RTP stream port handling needs to be done independently for AUDIO/VIDEO/IMAGE media types -- therefore their ranges must now be split per media type.
This also implies that the "-min_rtp_port" and "-max_rtp_port" command line parameters now have to be specified per media type:
-min_rtp_audio_port(equivalent to former "-min_rtp_port" for backward compatibility)-max_rtp_audio_port(equivalent to former "-max_rtp_port" for backward compatibility)-min_rtp_video_port-max_rtp_video_port-min_udp_image_port(FAX is typically transmitted using udptl)-max_udp_image_port(FAX is typically transmitted using udptl)Also note that port rotation support is made using [rtpstream_audio_port] and [rtpstream_video_port] -- NOT using [media_port] which is for the basic SIPP RTP echo functionality that is kept as-is by the current patch -- only RTPSTREAM code is really affected.
Port rotation is supported for BOTH RTP/SRTP media types (via additions in RTPSTREAM code).
Port rotation is supported for BOTH the UAC or UAS sides.
Port rotation can be performed by using the "+n" suffix with any "[rtpstream_audio_port]" or "[rtpstream_video_port]" XML syntax -- where n is an EVEN number (to respect RFC3551 specification for RTP port numbers).
Example for AUDIO RTP port rotation:
[rtpstream_audio_port+2]Example for VIDEO RTP port rotation:
[rtpstream_video_port+2]I've obviously fully successfully tested this port rotation with AUDIO and VIDEO RTPSTREAM scenarios -- see all included XML examples as necessary.
As for the (small) changes related to AUDIO/VIDEO/IMAGE PCAP_PLAY I am not aware of a proper way to test -- please advise.
I have not yet refactored this code to gather any of your suggestions before investing any further efforts.
Regards :)