[Asterisk-code-review] rtp: Add support for RTP extension negotiation and abs-send-... (asterisk[15])

[Richard Mudgett]

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

Change subject: rtp: Add support for RTP extension negotiation and abs-send-time.
......................................................................


Patch Set 2: Code-Review-1

(15 comments)

https://gerrit.asterisk.org/#/c/8883/2/include/asterisk/rtp_engine.h
File include/asterisk/rtp_engine.h:

https://gerrit.asterisk.org/#/c/8883/2/include/asterisk/rtp_engine.h@540
PS2, Line 540: 	/* Per the RFC 0 should not be used, so we treat it as an unsupported extension placeholder */
             : 	AST_RTP_EXTENSION_UNSUPPORTED = 0,
             : 	/* abs-send-time from https://tools.ietf.org/html/draft-alvestrand-rmcat-remb-03 */
             : 	AST_RTP_EXTENSION_ABS_SEND_TIME,
             : 	/* The maximum number of known RTP extensions */
Missing ! to start doxygen comment.  /*! */


https://gerrit.asterisk.org/#/c/8883/2/include/asterisk/rtp_engine.h@741
PS2, Line 741: 	/* The extension is not negotiated and is not flowing */
             : 	AST_RTP_EXTENSION_DIRECTION_NONE,
             : 	/* Send and receive */
Missing ! to start doxygen comment.  /*! */


https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c
File main/rtp_engine.c:

https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@732
PS2, Line 732: 	if (id == -1) {
Should we check this way instead of looking only for -1 to be safe?

if (id <= 0) {
} else {
}


https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@838
PS2, Line 838: 	struct rtp_extmap extmap_none = {
This should be declared static const struct rtp_extmap.


https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@875
PS2, Line 875: 	size_t count = 0;
Does not need to be initialized.  It is always set by AST_VECTOR_SIZE().


https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@894
PS2, Line 894: 	if (id <= AST_VECTOR_SIZE(&instance->extmap_unique_ids)) {
There seems to be a hole in the treatment of id: -1, 1 to max seems to be valid values.  What happens when zero is passed in?

if (0 < id && id <= AST_VECTOR_SIZE()) {
}


https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@910
PS2, Line 910: 	if (id <= AST_VECTOR_SIZE(&instance->extmap_unique_ids)) {
There seems to be a hole in the treatment of id: -1, 1 to max seems to be valid values.  What happens when zero is passed in?

if (0 < id && id <= AST_VECTOR_SIZE()) {
}


https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:

https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@161
PS2, Line 161: 	int idx = -1;
Should call this id instead since that is what is put in it?


https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@183
PS2, Line 183: 				if ((ast_rtp_instance_extmap_count(other_session_media->rtp) + 1) > idx) {
             : 					idx = ast_rtp_instance_extmap_count(other_session_media->rtp) + 1;
Rather than calling the function twice which gets and releases the instance lock.

other_id = ast_rtp_instance_extmap_count(other_session_media->rtp) + 1;
if (idx < other_id) {
  idx = other_id;
}


https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@1153
PS2, Line 1153: 	char tmp[256];
tmp is always a useless name.  extmap_value[]


https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@1214
PS2, Line 1214: 		char direction_str[10] = "", uri[64], attributes[64] = "";
It is best if you declare one variable per line.

I seem to recall that URI's can get quite large.  64 seems small.


https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@1245
PS2, Line 1245: 		if (sscanf(uri_str, "%63s %63s", uri, attributes) < 1) {
Since we own the uri_str buffer we can modify it and point uri and attributes to appropriate spots in the buffer rather than having the hard coded limits of the uri[] and attributes[].


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

https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c@4417
PS2, Line 4417: 		int hdrlen = 12, res, ice, ext = 0, abs_send_time_id;
It is best to declare one variable per line.


https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c@4424
PS2, Line 4424: 			/* 4 bytes for the shared information, 4 bytes for abs-send-time */
              : 			hdrlen += 8;
This seems wrong.  We are adding two uint32's plus a uint24 for a total of 4 + 4 + 3 = 11.


https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c@5309
PS2, Line 5309: 	unsigned int now_msw = 0, now_lsw = 0;
It is best if one variable declaration per line.



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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I508deac557867b1e27fc7339be890c8018171588
Gerrit-Change-Number: 8883
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 03 May 2018 20:53:44 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180503/bf1ee3cb/attachment-0001.html>