[Asterisk-code-review] codec negotiation: add incoming_call_offer_prefs option (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13842 )

Change subject: codec negotiation: add incoming_call_offer_prefs option
......................................................................


Patch Set 1: Code-Review-1

(12 comments)

https://gerrit.asterisk.org/c/asterisk/+/13842/1/configs/samples/pjsip.conf.sample 
File configs/samples/pjsip.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/configs/samples/pjsip.conf.sample@801 
PS1, Line 801: ;incoming_call_offer_pref= ; Sets the preferred codecs, and order to use between
Tabs have gone a bit bonkers here.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/doc/CHANGES-staging/res_pjsip_incoming_call_offer_pref.txt 
File doc/CHANGES-staging/res_pjsip_incoming_call_offer_pref.txt:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/doc/CHANGES-staging/res_pjsip_incoming_call_offer_pref.txt@7 
PS1, Line 7: offer, and those set in the configuration.
Extend this to include a better explanation with examples.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip.h 
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip.h@516 
PS1, Line 516: 	AST_SIP_CALL_CODEC_PREF_LOCAL,
Document the meaning of these.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session.h 
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session.h@34 
PS1, Line 34: #include "asterisk/res_pjsip_session_caps.h"
Any reason to not just use a forward declaration instead?


https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h 
File include/asterisk/res_pjsip_session_caps.h:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h@27 
PS1, Line 27:  * \brief Allocate a sip session capabilities object.
SIP is traditionally capitalized :D


https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h@40 
PS1, Line 40:  */
Document that this will replace any already set or built capabilities.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h@61 
PS1, Line 61:  * Creates a set of joint capabilities between the given remote capabilities,
            :  * and pre-configured ones.
This API feels weird, because generally these things return a unique capabilities structure that isn't persisted. I think it's the use of "build" that makes me think that. Perhaps "update" would be better? An explanation that it is stored away would also help.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip.c 
File res/res_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip.c@949 
PS1, Line 949: 				<configOption name="rtp_keepalive">
             : 					<synopsis>Number of seconds between RTP comfort noise keepalive packets.</synopsis>
             : 					<description><para>
             : 						At the specified interval, Asterisk will send an RTP comfort noise frame. This may
             : 						be useful for situations where Asterisk is behind a NAT or firewall and must keep
             : 						a hole open in order to allow for media to arrive at Asterisk.
             : 					</para></description>
             : 				</configOption>
You've doubled this documentation.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip/pjsip_configuration.c 
File res/res_pjsip/pjsip_configuration.c:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip/pjsip_configuration.c@1137 
PS1, Line 1137: 			if (i == AST_SIP_CALL_CODEC_PREF_LOCAL_LIMIT ||
Explain why this is here


https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_sdp_rtp.c 
File res/res_pjsip_sdp_rtp.c:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_sdp_rtp.c@420 
PS1, Line 420: 	}
This should return now since this has failed.


https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_session/pjsip_session_caps.c 
File res/res_pjsip_session/pjsip_session_caps.c:

https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_session/pjsip_session_caps.c@36 
PS1, Line 36: static void log_caps(int level, const char *file, int line, const char *function,
Is there any way we can get additional information in here to tie it back to a channel or endpoint?


https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_session/pjsip_session_caps.c@138 
PS1, Line 138: 		/* Save the most preferred one */
Oh! I'd add a comment here that since a stream only carries a single media type there's no need to worry about the type or dealing with multiple, so just pulling the first one really will do the job.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13842
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I35e7a2a0c236cfb6bd9cdf89539f57a1ffefc76f
Gerrit-Change-Number: 13842
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 11:35:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200227/ea0b48b7/attachment-0001.html>