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

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5138 )

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


Patch Set 2: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/5138/2/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

PS2, Line 675: 	/*! Use RTCP-MUX */
             : 	unsigned int rtcp_mux;
This breaks ABI on v13 and v14 because the struct is inserted into ast_sip_endpoint.  It is perfectly acceptable and preferred on master of course.

I think we need to break up ast_sip_endpoint on master to have pointers to independently allocated structs like this instead of directly inserting the structs into ast_sip_endpoint.  The drawback is going to be how sorcery/config-framework will have to deal with the options.


https://gerrit.asterisk.org/#/c/5138/2/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 672: 		ast_mutex_lock(&rtp->lock);
This is left locked.


PS2, Line 4560: 	pt = packet[1] & 0x7F;
              : 	if (pt >= 64 && pt <= 95) {
              : 		return 1;
              : 	}
This isn't testing the marker bit to distinguish between RTP and RTCP.


Line 4572: 	unsigned char rtpdata[8192 + AST_FRIENDLY_OFFSET];
An 8k stack variable?  Is there not a better place for a buffer?


PS2, Line 4897: 	rtp->f.data.ptr = rtp->rawdata + AST_FRIENDLY_OFFSET;
              : 	memcpy(rtp->f.data.ptr, write_area + hdrlen , rtp->f.datalen);
Now we copy the RTP packet contents for every packet.  This sounds expensive.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If46a93ba1282418d2803e3fd7869374da8b77ab5
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
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-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-HasComments: Yes