[Asterisk-code-review] res_pjsip: Adjust outgoing offer call pref. (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Tue Oct 13 11:14:27 CDT 2020
Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/15047 )
Change subject: res_pjsip: Adjust outgoing offer call pref.
......................................................................
res_pjsip: Adjust outgoing offer call pref.
This changes the outgoing offer call preference
default option to match the behavior of previous
versions of Asterisk.
The additional advanced codec negotiation options
have also been removed from the sample configuration
and marked as reserved for future functionality in
XML documentation.
The codec preference options have also been fixed to
enforce local codec configuration.
ASTERISK-29109
Change-Id: Iad19347bd5f3d89900c15ecddfebf5e20950a1c2
---
M configs/samples/pjsip.conf.sample
M res/res_pjsip.c
M res/res_pjsip/pjsip_configuration.c
M res/res_pjsip_session/pjsip_session_caps.c
M tests/test_res_pjsip_session_caps.c
5 files changed, 36 insertions(+), 77 deletions(-)
Approvals:
George Joseph: Looks good to me, but someone else must approve
Kevin Harwell: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Approved for Submit
diff --git a/configs/samples/pjsip.conf.sample b/configs/samples/pjsip.conf.sample
index 506583e..cded743 100644
--- a/configs/samples/pjsip.conf.sample
+++ b/configs/samples/pjsip.conf.sample
@@ -828,69 +828,17 @@
; local - Include all codecs in the local list that
; are also in the remote list preserving the local
; order.
- ; local_merge - Include all codecs in BOTH lists
- ; preserving the local list order. Codes in the
- ; remote list not in the local list will be placed
- ; at the end of the joint list.
+ ; local_merge - Include all codecs in the local list
+ ; preserving the local order.
; local_first - Include only the first codec in the
; local list.
; remote - Include all codecs in the remote list that
; are also in the local list preserving remote list
- ; order. (default)
- ; remote_merge - Include all codecs in BOTH lists
- ; preserving the remote list order. Codes in the
- ; local list not in the remote list will be placed
- ; at the end of the joint list.
- ; remote_first - Include only the first codec in
- ; the remote list.
-;codec_prefs_incoming_offer=; This is a string that describes how the codecs
- ; specified on an incoming SDP offer (pending) are
- ; reconciled with the codecs specified on an endpoint
- ; (configured) before being sent to the Asterisk core.
- ; The string actually specifies 4 name:value pair
- ; parameters separated by commas. Whitespace is
- ; ignored and they may be specified in any order.
- ; prefer: <pending | configured>,
- ; operation: <intersect | only_preferred
- ; | only_nonpreferred>,
- ; keep: <first | all>,
- ; transcode: <allow | prevent>
-;codec_prefs_outgoing_offer=; This is a string that describes how the codecs
- ; specified in the topology that comes from the
- ; Asterisk core (pending) are reconciled with the
- ; codecs specified on an endpoint (configured)
- ; when sending an SDP offer.
- ; The string actually specifies 4 name:value pair
- ; parameters separated by commas. Whitespace is
- ; ignored and they may be specified in any order.
- ; prefer: <pending | configured>,
- ; operation: <intersect | union
- ; | only_preferred | only_nonpreferred>,
- ; keep: <first | all>,
- ; transcode: <allow | prevent>
-;codec_prefs_incoming_answer=; This is a string that describes how the codecs
- ; specified in an incoming SDP answer (pending)
- ; are reconciled with the codecs specified on an
- ; endpoint (configured) when receiving an SDP
- ; answer.
- ; The string actually specifies 4 name:value pair
- ; parameters separated by commas. Whitespace is
- ; ignored and they may be specified in any order.
- ; prefer: <pending | configured>,
- ; operation: <intersect | union
- ; | only_preferred | only_nonpreferred>,
- ; keep: <first | all>
-;codec_prefs_outgoing_answer=; This is a string that describes how the codecs
- ; that come from the core (pending) are reconciled
- ; with the codecs specified on an endpoint
- ; (configured) when sending an SDP answer.
- ; The string actually specifies 4 name:value pair
- ; parameters separated by commas. Whitespace is
- ; ignored and they may be specified in any order.
- ; prefer: <pending | configured>,
- ; operation: <intersect | union
- ; | only_preferred | only_nonpreferred>,
- ; keep: <first | all>
+ ; order.
+ ; remote_merge - Include all codecs in the local list
+ ; preserving the remote list order. (default)
+ ; remote_first - Include only the first codec in the
+ ; remote list that is also in the local list.
;preferred_codec_only=no ; Respond to a SIP invite with the single most
; preferred codec rather than advertising all joint
; codec capabilities. This limits the other side's
diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 9d3a6c2..cfc97b6 100644
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -111,6 +111,7 @@
on an endpoint (configured) before being sent to the Asterisk core.
The string actually specifies 4 <literal>name:value</literal> pair parameters
separated by commas. Whitespace is ignored and they may be specified in any order.
+ Note that this option is reserved for future functionality.
</para>
<para>
@@ -171,6 +172,7 @@
endpoint (configured) when sending an SDP offer.
The string actually specifies 4 <literal>name:value</literal> pair parameters
separated by commas. Whitespace is ignored and they may be specified in any order.
+ Note that this option is reserved for future functionality.
</para>
<para>
@@ -232,6 +234,8 @@
when receiving an SDP answer.
The string actually specifies 4 <literal>name:value</literal> pair parameters
separated by commas. Whitespace is ignored and they may be specified in any order.
+ Note that this option is reserved for future functionality.
+
</para>
<para>
Parameters:
@@ -288,6 +292,8 @@
when sending an SDP answer.
The string actually specifies 4 <literal>name:value</literal> pair parameters
separated by commas. Whitespace is ignored and they may be specified in any order.
+ Note that this option is reserved for future functionality.
+
</para>
<para>
Parameters:
@@ -1214,7 +1220,7 @@
</enumlist>
</description>
</configOption>
- <configOption name="outgoing_call_offer_pref" default="local">
+ <configOption name="outgoing_call_offer_pref" default="remote_merge">
<synopsis>Preferences for selecting codecs for an outgoing call.</synopsis>
<description>
<para>Based on this setting, a joint list of preferred codecs between
@@ -1227,24 +1233,20 @@
preserving the local order.
</para></enum>
<enum name="local_merge"><para>
- Include all codecs in BOTH lists preserving the local order.
- Remote codecs not in the local list will be placed at the end
- of the joint list.
+ Include all codecs in the local list preserving the local order.
</para></enum>
<enum name="local_first"><para>
Include only the first codec in the local list.
</para></enum>
<enum name="remote"><para>
Include all codecs in the remote list that are also in the local list
- preserving the remote order. (default)
+ preserving the remote order.
</para></enum>
<enum name="remote_merge"><para>
- Include all codecs in BOTH lists preserving the remote order.
- Local codecs not in the remote list will be placed at the end
- of the joint list.
+ Include all codecs in the local list preserving the remote order. (default)
</para></enum>
<enum name="remote_first"><para>
- Include only the first codec in the remote list.
+ Include only the first codec in the remote list that is also in the local list.
</para></enum>
</enumlist>
</description>
diff --git a/res/res_pjsip/pjsip_configuration.c b/res/res_pjsip/pjsip_configuration.c
index 344f319..80bad02 100644
--- a/res/res_pjsip/pjsip_configuration.c
+++ b/res/res_pjsip/pjsip_configuration.c
@@ -2138,7 +2138,7 @@
ast_sorcery_object_field_register(sip_sorcery, "endpoint", "ignore_183_without_sdp", "no", OPT_BOOL_T, 1, FLDSET(struct ast_sip_endpoint, ignore_183_without_sdp));
ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "incoming_call_offer_pref", "local",
call_offer_pref_handler, incoming_call_offer_pref_to_str, NULL, 0, 0);
- ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "outgoing_call_offer_pref", "remote",
+ ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "outgoing_call_offer_pref", "remote_merge",
call_offer_pref_handler, outgoing_call_offer_pref_to_str, NULL, 0, 0);
ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "codec_prefs_incoming_offer",
"prefer: pending, operation: intersect, keep: all, transcode: allow",
diff --git a/res/res_pjsip_session/pjsip_session_caps.c b/res/res_pjsip_session/pjsip_session_caps.c
index ca7ef44..d44f1a6 100644
--- a/res/res_pjsip_session/pjsip_session_caps.c
+++ b/res/res_pjsip_session/pjsip_session_caps.c
@@ -68,34 +68,43 @@
{
struct ast_format_cap *joint = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
struct ast_format_cap *local_filtered = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
+ struct ast_format_cap *remote_filtered = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
- if (!joint || !local_filtered) {
+ if (!joint || !local_filtered || !remote_filtered) {
ast_log(LOG_ERROR, "Failed to allocate %s call offer capabilities\n",
ast_codec_media_type2str(media_type));
ao2_cleanup(joint);
ao2_cleanup(local_filtered);
+ ao2_cleanup(remote_filtered);
return NULL;
}
ast_format_cap_append_from_cap(local_filtered, local, media_type);
+ /* Remote should always be a subset of local, as local is what defines the underlying
+ * permitted formats.
+ */
+ ast_format_cap_get_compatible(remote, local_filtered, remote_filtered);
+
if (ast_sip_call_codec_pref_test(codec_pref, LOCAL)) {
if (ast_sip_call_codec_pref_test(codec_pref, INTERSECT)) {
- ast_format_cap_get_compatible(local_filtered, remote, joint); /* Get common, prefer local */
+ ast_format_cap_get_compatible(local_filtered, remote_filtered, joint); /* Get common, prefer local */
} else {
ast_format_cap_append_from_cap(joint, local_filtered, media_type); /* Add local */
- ast_format_cap_append_from_cap(joint, remote, media_type); /* Then remote */
+ ast_format_cap_append_from_cap(joint, remote_filtered, media_type); /* Then remote */
}
} else {
if (ast_sip_call_codec_pref_test(codec_pref, INTERSECT)) {
- ast_format_cap_get_compatible(remote, local_filtered, joint); /* Get common, prefer remote */
+ joint = remote_filtered; /* Get common, prefer remote - as was done when filtering initially */
+ remote_filtered = NULL;
} else {
- ast_format_cap_append_from_cap(joint, remote, media_type); /* Add remote */
+ ast_format_cap_append_from_cap(joint, remote_filtered, media_type); /* Add remote */
ast_format_cap_append_from_cap(joint, local_filtered, media_type); /* Then local */
}
}
ao2_ref(local_filtered, -1);
+ ao2_cleanup(remote_filtered);
if (ast_format_cap_empty(joint)) {
return joint;
diff --git a/tests/test_res_pjsip_session_caps.c b/tests/test_res_pjsip_session_caps.c
index 54438f5..a97c214 100644
--- a/tests/test_res_pjsip_session_caps.c
+++ b/tests/test_res_pjsip_session_caps.c
@@ -147,11 +147,11 @@
ast_test_status_update(test, "Testing outgoing expected pass\n");
RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "local", 1, "alaw,g722", AST_TEST_PASS);
RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "local_first", 1, "alaw", AST_TEST_PASS);
- RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "local_merge", 1, "ulaw,alaw,g722,g729", AST_TEST_PASS);
+ RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "local_merge", 1, "ulaw,alaw,g722", AST_TEST_PASS);
RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "remote", 1, "g722,alaw", AST_TEST_PASS);
RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "remote_first", 1, "g722", AST_TEST_PASS);
- RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "remote_merge", 1, "g722,g729,alaw,ulaw", AST_TEST_PASS);
- RUN_CREATE_JOINT("!all", "g722,g729,alaw", "remote_merge", 1, "g722,g729,alaw", AST_TEST_PASS);
+ RUN_CREATE_JOINT("ulaw,alaw,g722", "g722,g729,alaw", "remote_merge", 1, "g722,alaw,ulaw", AST_TEST_PASS);
+ RUN_CREATE_JOINT("!all", "g722,g729,alaw", "remote_merge", 1, "nothing", AST_TEST_PASS);
return rc >= 1 ? AST_TEST_FAIL : AST_TEST_PASS;
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15047
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iad19347bd5f3d89900c15ecddfebf5e20950a1c2
Gerrit-Change-Number: 15047
Gerrit-PatchSet: 3
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20201013/204e7a89/attachment-0001.html>
More information about the asterisk-code-review
mailing list