[Asterisk-code-review] core & res_pjsip: Improve topology change handling. (asterisk[20])
Friendly Automation
asteriskteam at digium.com
Tue Nov 29 08:23:58 CST 2022
Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/19585 )
Change subject: core & res_pjsip: Improve topology change handling.
......................................................................
core & res_pjsip: Improve topology change handling.
This PR contains two relatively separate changes in channel.c and
res_pjsip_session.c which ensure that topology changes are not ignored
in cases where they should be handled.
For channel.c:
The function ast_channel_request_stream_topology_change only triggers a
stream topology request change indication, if the channel's topology
does not equal the requested topology. However, a channel could be in a
state where it is currently "negotiating" a new topology but hasn't
updated it yet, so the topology request change would be lost. Channels
need to be able to handle such situations internally and stream
topology requests should therefore always be passed on.
In the case of chan_pjsip for example, it queues a session refresh
(re-INVITE) if it is currently in the middle of a transaction or has
pending requests (among other reasons).
Now, ast_channel_request_stream_topology_change always indicates a
stream topology request change even if the requested topology equals the
channel's topology.
For res_pjsip_session.c:
The function resolve_refresh_media_states does not process stream state
changes if the delayed active state differs from the current active
state. I.e. if the currently active stream state has changed between the
time the sip session refresh request was queued and the time it is being
processed, the session refresh is ignored. However, res_pjsip_session
contains logic that ensures that session refreshes are queued and
re-queued correctly if a session refresh is currently not possible. So
this check is not necessary and led to some session refreshes being
lost.
Now, a session refresh is done even if the delayed active state differs
from the current active state and it is checked whether the delayed
pending state differs from the current active - because that means a
refresh is necessary.
Further, the unit test of resolve_refresh_media_states was adapted to
reflect the new behavior. I.e. the changes to delayed pending are
prioritized over the changes to current active because we want to
preserve the original intention of the pending state.
ASTERISK-30184
Change-Id: Icd0703295271089057717006730b555b9a1d4e5a
---
M main/channel.c
M res/res_pjsip_session.c
2 files changed, 62 insertions(+), 19 deletions(-)
Approvals:
George Joseph: Looks good to me, approved
Friendly Automation: Approved for Submit
diff --git a/main/channel.c b/main/channel.c
index 97ba0f8..8fa4509 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -11163,15 +11163,6 @@
return -1;
}
- if (ast_stream_topology_equal(ast_channel_get_stream_topology(chan), topology)) {
- ast_debug(2, "%s: Topologies already match. Current: %s Requested: %s\n",
- ast_channel_name(chan),
- ast_str_tmp(256, ast_stream_topology_to_str(ast_channel_get_stream_topology(chan), &STR_TMP)),
- ast_str_tmp(256, ast_stream_topology_to_str(topology, &STR_TMP)));
- ast_channel_unlock(chan);
- return 0;
- }
-
ast_channel_internal_set_stream_topology_change_source(chan, change_source);
res = ast_channel_tech(chan)->indicate(chan, AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE, topology, sizeof(topology));
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index c5890e0..a7bde0d 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1935,15 +1935,7 @@
/* All the same state, no need to update. */
SCOPE_EXIT_EXPR(continue, "%s: All in the same state so nothing to do\n", session_name);
}
- if (da_state != ca_state) {
- /*
- * Something set the CA state between the time this request was queued
- * and now. The CA state wins so we don't do anything.
- */
- SCOPE_EXIT_EXPR(continue, "%s: Ignoring request to change state from %s to %s\n",
- session_name, ast_stream_state2str(ca_state), ast_stream_state2str(dp_state));
- }
- if (dp_state != da_state) {
+ if (dp_state != ca_state) {
/* DP needs to update the state */
ast_stream_set_state(np_stream, dp_state);
SCOPE_EXIT_EXPR(continue, "%s: Changed NP stream state from %s to %s\n",
@@ -5879,6 +5871,7 @@
test_media_add(expected_pending_state, "audio", AST_MEDIA_TYPE_AUDIO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo1", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
+ test_media_add(expected_pending_state, "myvideo2", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo3", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
CHECKER();
@@ -5917,8 +5910,9 @@
test_media_add(expected_pending_state, "audio", AST_MEDIA_TYPE_AUDIO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo1", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
- test_media_add(expected_pending_state, "myvideo3", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
+ test_media_add(expected_pending_state, "myvideo2", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo4", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
+ test_media_add(expected_pending_state, "myvideo3", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
CHECKER();
RESET_STATE(7);
@@ -5964,6 +5958,7 @@
test_media_add(expected_pending_state, "audio", AST_MEDIA_TYPE_AUDIO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo1", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
+ test_media_add(expected_pending_state, "myvideo2", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo3", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo4", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
CHECKER();
@@ -5984,6 +5979,8 @@
test_media_add(current_active_state, "myvideo2", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_REMOVED, -1);
test_media_add(expected_pending_state, "audio", AST_MEDIA_TYPE_AUDIO, AST_STREAM_STATE_SENDRECV, -1);
+ test_media_add(expected_pending_state, "myvideo1", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
+ test_media_add(expected_pending_state, "myvideo2", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo3", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
test_media_add(expected_pending_state, "myvideo4", AST_MEDIA_TYPE_VIDEO, AST_STREAM_STATE_SENDRECV, -1);
CHECKER();
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/19585
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 20
Gerrit-Change-Id: Icd0703295271089057717006730b555b9a1d4e5a
Gerrit-Change-Number: 19585
Gerrit-PatchSet: 2
Gerrit-Owner: Maximilian Fridrich <m.fridrich at commend.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221129/ebf9aa05/attachment-0001.html>
More information about the asterisk-code-review
mailing list