[Asterisk-code-review] Add rtcp-mux support (asterisk[14])

[Mark Michelson]

Mark Michelson has posted comments on this change. ( https://gerrit.asterisk.org/5139 )

Change subject: Add rtcp-mux support
......................................................................


Patch Set 1:

(2 comments)

https://gerrit.asterisk.org/#/c/5139/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 4571: 	unsigned char rtpdata[sizeof(rtp->rawdata)];
> Why the change away from the usage of rtp->rawdata?
Good question. The old logic was that ast_rtp_read() would read data into the rtp->rawdata. Meanwhile, ast_rtcp_read() read data into a stack-allocated rtcpdata buffer. Why did ast_rtcp_read() do that? It's because depending on the type of RTCP packet encountered, ast_rtcp_read() would generate a frame to return. This frame's data would be placed in the rtp->rawdata.

So now the time comes to add rtcp-mux. ast_rtp_read() could either be reading RTP data or RTCP data. Either way, you need to do the read into the same area. If we end up reading RTP data into rtp->rawdata, everything's cool. However, if we read RTCP data into rtp->rawdata, we could have a problem. ast_rtcp_read() (or ast_rtcp_interpret(), as it's changed in this patch) could end up either doing an overlapping memcpy or overwriting data that it needs to get to when interpreting a compound RTCP packet.


PS1, Line 5168: 			if (rtp->rtcp->type == AST_RTP_INSTANCE_RTCP_STANDARD) {
              : 				ast_sockaddr_set_port(&rtp->rtcp->them, ast_sockaddr_port(addr) + 1);
              : 
              : 				/* Update the local RTCP address with what is being used */
              : 				ast_sockaddr_set_port(&local, ast_sockaddr_port(&local) + 1);
              : 			}
> Do we use this data anywhere and if so, should we set it even when muxed? (
See the line two above where you commented (line 5166). It sets rtcp->them to be the same as the remote RTP address.


-- 
To view, visit https://gerrit.asterisk.org/5139
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If46a93ba1282418d2803e3fd7869374da8b77ab5
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes