core,app_dial,pjsip: Implement Advanced Codec Negotiation (ACN) #285

maximilianfridrich · 2023-08-30T09:25:10Z

Advanced Codec Negotiation is now implemented for the dial application, pjsip channels and local channels.

Resolves: #223

UpgradeNote: The two pjsip.conf options "incoming_call_offer_pref" and "outgoing_call_offer_pref" have been removed. Instead, the new options "codec_prefs_incoming_answer", "codec_prefs_incoming_offer", "codec_prefs_outgoing_answer", "codec_prefs_outgoing_offer" must be used. Even if the old settings were not used, this could break existing deployments as the default ACN options are now in use and they might behave differently in some call flows (e.g. by default, pending codecs are always preferred, not the endpoint configuration).

UserNote: The Advanced Codec Negotiation feature is now implemented.

maximilianfridrich · 2023-08-30T09:25:28Z

cherry-pick-to: 21
cherry-pick-to: 20
cherry-pick-to: 18

jcolp · 2023-08-30T09:31:33Z

I haven't looked at the code, just the upgrade note, but the options can not be removed. They must continue to exist.

maximilianfridrich · 2023-08-30T09:33:48Z

This PR must be tested with the corresponding testsuite PR which adds 48 tests to test the codec negotiation:

pjsip: Add tests for Advanced Codec Negotiation (ACN) testsuite#20

Further, 10 tests were adopted to reflect the new behavior. For those tests, pjsip.conf had to be adopted to use the new ACN settings. The default settings prefer the pending stream while these tests expect the configured settings to be preferred.

maximilianfridrich · 2023-08-30T09:37:32Z

the options can not be removed. They must continue to exist.

@jcolp And they must continue to work? Or silently be ignored like the ACN settings were? I'm not sure if I see a way for both settings to co-exist without interfering with each other.

jcolp · 2023-08-30T09:46:20Z

Yes. We can not remove options in minor versions, or alter their behavior, unless there is a critical reason why - such as a security issue. This does not rise to that level.

jcolp · 2023-08-30T09:48:08Z

For example, if the options are kept but default to empty in the code we can determine that they are actually unset. Default behavior should remain the same in that case even if the options were removed. If they are set then those settings take priority. If unset then the other options take priority.

Default behavior with this change should match previous versions
If the incoming_* and outgoing_* options are set by the user, then they take priority

maximilianfridrich · 2023-08-30T09:51:57Z

We can not remove options in minor versions, or alter their behavior

Makes sense, I probably should have checked with the maintainers first. So would it be an option for Asterisk 21?

jcolp · 2023-08-30T09:52:29Z

Asterisk 21, once branched, can no longer receive breaking changes.

jcolp · 2023-08-30T09:54:00Z

I'm sticking to the policy that breaking changes are for standard releases, which next would be 23. If this change can be altered to maintain backwards compatibility then it would be eligible for all.

maximilianfridrich · 2023-08-30T09:55:18Z

Ok, then I will look into your suggestion and try to find a way to not change the existing behavior and not remove any settings.

Is it possible to mark this PR as a draft while I do that?

jcolp · 2023-08-30T09:58:32Z

I've converted it to a draft.

maximilianfridrich · 2023-08-30T11:03:55Z

Default behavior with this change should match previous versions

If the incoming_* and outgoing_* options are set by the user, then they take priority

Consider the case where neither the "old" options incoming_call_offer_pref and outgoing_call_offer_pref nor the "new" codec_prefs_* options are set. As you described it, since the old options are unset, the new settings are used. And in order to not change the default behavior, I must change the defaults of two of the new ACN options:

codec_prefs_incoming_offer = prefer: configured, operation: intersect, keep: all, transcode: allow
codec_prefs_incoming_answer = prefer: configured, operation: intersect, keep: all, transcode: allow

They must be set to prefer: configured, this is the only way the requirements can be met. So far, the documentation (and code) had prefer: pending as default. The ACN settings had no effect so far, so I think this should be alright.

gtjoseph · 2023-08-30T11:55:01Z

Thanks for picking this up!!!
I'm going to try and set some time aside to review in detail.

maximilianfridrich · 2023-08-30T12:01:40Z

@gtjoseph Thank you! I am now incorporating Joshua's remarks and I noticed that some fax/pjsip tests failed (which I didn't check). So this is still a WIP, but we have confirmed that the ACN is working as expected (e.g. see the linked testsuite PR and we have done manual testing as well).

maximilianfridrich · 2023-08-30T12:17:00Z

If the incoming_* and outgoing_* options are set by the user, then they take priority.

To be more precise, I think the logic should be:

As soon as one of incoming_call_offer_pref or outgoing_call_offer_pref is set, the ACN options are ignored.

I think everything else would lead to maximum confusion. I will state this in the docs for the settings.

maximilianfridrich · 2023-08-31T13:40:50Z

Update: We can not really invest more time into making this patch also work with the old settings.

@jcolp Is the removal of the incoming_call_offer_pref and outgoing_call_offer_pref an option for the next Asterisk major version release? If so, how can we go about this? Should we close this PR and re-open it at a better time?

We are confident that this patch implements ACN as it is supposed to, the only missing piece seems to be the failing fax tests.

InterLinked1 · 2023-08-31T13:45:28Z

@jcolp Is the removal of the incoming_call_offer_pref and outgoing_call_offer_pref an option for the next Asterisk major version release? If so, how can we go about this? Should we close this PR and re-open it at a better time?

There is a waiting-for-standard-release-development-cycle label for this purpose, that could be applied. But I'm not sure contributors can add it themselves.

jcolp · 2023-08-31T13:53:16Z

I've added it.

btriller · 2023-09-11T06:57:29Z

This leaks some memory. Following patch fixes it. Unsure, if this is the correct approach though. @maximilianfridrich

diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 66ea75d0e0..e1d003180f 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -2917,6 +2917,10 @@ static int request(void *obj)
 		SCOPE_EXIT_RTN_VALUE(-1, "Couldn't create session\n");
 	}
 
+	if (resolved_top) {
+		ast_stream_topology_free(resolved_top);
+	}
+
 	req_data->session = session;
 
 	SCOPE_EXIT_RTN_VALUE(0);
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 282878adb8..0da14eca13 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -941,6 +941,7 @@ static int handle_incoming_sdp(struct ast_sip_session *session, const pjmedia_sd
 					ast_trace(-1, "%s: No common codecs between incoming SDP offer and endpoint configuration.\n", ast_sip_session_get_name(session));
 				} else {
 					stream = ast_stream_clone(resolved_stream, stream_name);
+					ast_stream_free(resolved_stream);
 				}
 			} else {
 				/* This can happen when the stream is marked as removed.

maximilianfridrich · 2023-09-12T07:22:22Z

Thank you for looking at it @btriller. That seems right. I declared both variables with the RAII_VAR macro now, that seems to be the best way. Not sure how this one slipped through and wasn't caught by our automated tests (which we ran with valgrind). I have updated the patch.

Also, please be aware that this patch as it is now breaks some of the fax/pjsip tests. The failing pjsip tests on GitHub are due to it not running it with the corresponding testsuite PR.

btriller · 2023-09-22T09:00:46Z

There's another issue, that leads to no audio, if a client sends more streams than endpoint's capability. See #344 @maximilianfridrich

maximilianfridrich · 2023-09-22T09:08:13Z

I'm not sure if I can follow. Could you please describe the scenario in more detail (endpoint config, and who answers/offers what)?

Update: Maybe all you need is operation: union in the correct codec_prefs_* setting.

btriller · 2023-11-06T17:37:26Z

res/res_pjsip_session.c

@@ -5401,6 +5611,8 @@ static void session_inv_on_create_offer(pjsip_inv_session *inv, pjmedia_sdp_sess
 		SCOPE_EXIT_RTN("%s: create offer failed\n", ast_sip_session_get_name(session));
 	}

+	ast_sip_session_media_state_reset(session->pending_media_state);


This seems to cause no audio with all_codecs_on_empty_reinvite=yes when a UA renegotiates a different codec. See [1] for reproducing this issue. Seems to be fixed with

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c index 5486a97040..72b4130a40 100644 --- a/res/res_pjsip_session.c +++ b/res/res_pjsip_session.c @@ -5594,25 +5594,23 @@ static void session_inv_on_create_offer(pjsip_inv_session *inv, pjmedia_sdp_sess } } - if (inv->neg) { - if (pjmedia_sdp_neg_was_answer_remote(inv->neg)) { - pjmedia_sdp_neg_get_active_remote(inv->neg, &previous_sdp); - } else { - pjmedia_sdp_neg_get_active_local(inv->neg, &previous_sdp); - } - } - if (ignore_active_stream_topology) { offer = create_local_sdp(inv, session, NULL, 1); } else { + if (inv->neg) { + if (pjmedia_sdp_neg_was_answer_remote(inv->neg)) { + pjmedia_sdp_neg_get_active_remote(inv->neg, &previous_sdp); + } else { + pjmedia_sdp_neg_get_active_local(inv->neg, &previous_sdp); + } + } offer = create_local_sdp(inv, session, previous_sdp, 0); + ast_sip_session_media_state_reset(session->pending_media_state); } if (!offer) { SCOPE_EXIT_RTN("%s: create offer failed\n", ast_sip_session_get_name(session)); } - ast_sip_session_media_state_reset(session->pending_media_state); - ast_queue_unhold(session->channel); /*

[1] acn_reinvite_without_sdp.patch

I don't doubt it, but the provided testcase passes for me even without your suggested patch. Is it testing exactly the scenario you described?

Negotiating codecs works fine without the patch but there's no audio (no RTP sent) after negotiating a new codec. I don't know how to verify that RTP is sent (maybe it's possible to check for RTP AMI events?). IIRC without the patch there's following log message

res_pjsip_sdp_rtp.c:487 set_caps: No joint capabilities for 'audio' media stream between our configuration((alaw)) and incoming SDP((g722))

Makes sense, maybe I can find a way to test it. Thank you for clarifying.

btriller · 2023-11-08T17:30:37Z

There's another memory leak. Please check following patch. @maximilianfridrich

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 5486a97040..cbbf6d05c4 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1225,8 +1225,8 @@ static int handle_negotiated_sdp(struct ast_sip_session *session, const pjmedia_
 		for (index = 0; index < local->media_count; index++) {
 			RAII_VAR(struct ast_format_cap *, remote_caps, NULL, ao2_cleanup);
 			RAII_VAR(struct ast_stream_topology *, remote_top, NULL, ao2_cleanup);
+			RAII_VAR(struct ast_stream *, resolved_stream, NULL, ast_stream_free);
 			struct ast_stream *remote_ast_stream;
-			struct ast_stream *resolved_stream;
 			struct ast_stream *configured_stream = ast_stream_topology_get_stream(session->pending_media_state->topology, index);
 			int skip_stream_setting = 0;

btriller · 2023-11-09T20:50:34Z

There's another memory leak. Please check following patch. @maximilianfridrich

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 5486a97040..cbbf6d05c4 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1225,8 +1225,8 @@ static int handle_negotiated_sdp(struct ast_sip_session *session, const pjmedia_
 		for (index = 0; index < local->media_count; index++) {
 			RAII_VAR(struct ast_format_cap *, remote_caps, NULL, ao2_cleanup);
 			RAII_VAR(struct ast_stream_topology *, remote_top, NULL, ao2_cleanup);
+			RAII_VAR(struct ast_stream *, resolved_stream, NULL, ast_stream_free);
 			struct ast_stream *remote_ast_stream;
-			struct ast_stream *resolved_stream;
 			struct ast_stream *configured_stream = ast_stream_topology_get_stream(session->pending_media_state->topology, index);
 			int skip_stream_setting = 0;

Former patch leads to crashes. Following seams more appropriate

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 5486a97040..9bfe47df30 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1259,6 +1259,7 @@ static int handle_negotiated_sdp(struct ast_sip_session *session, const pjmedia_
 				&session->endpoint->media.codec_prefs_incoming_answer,
 				NULL);
 			if (!resolved_stream || ast_format_cap_empty(ast_stream_get_formats(resolved_stream))) {
+				ast_stream_free(resolved_stream); /* Can handle NULL */
 				if (session->endpoint->media.codec_prefs_incoming_answer.transcode == CODEC_NEGOTIATION_TRANSCODE_ALLOW) {
 					ast_log(LOG_DEBUG, "%s: No common codecs between channels and transcoding allowed.\n",
 						ast_sip_session_get_name(session));

maximilianfridrich · 2023-11-15T08:42:58Z

Former patch leads to crashes. Following seams more appropriate

That looks good, thank you for the find!

Advanced Codec Negotiation is now implemented for the dial application, pjsip channels and local channels. Resolves: asterisk#223 UpgradeNote: The two pjsip.conf options "incoming_call_offer_pref" and "outgoing_call_offer_pref" have been removed. Instead, the new options "codec_prefs_incoming_answer", "codec_prefs_incoming_offer", "codec_prefs_outgoing_answer", "codec_prefs_outgoing_offer" must be used. Even if the old settings were not used, this could break existing deployments as the default ACN options are now in use and they might behave differently in some call flows (e.g. by default, pending codecs are always preferred, not the endpoint configuration). UserNote: The Advanced Codec Negotiation feature is now implemented.

SirDMZ · 2024-04-09T01:00:14Z

Hey Maximilianfridrich,

Have you got a version of this patch for version 20 available?

maximilianfridrich · 2024-04-09T11:18:14Z

@SirDMZ I tested the patches with Asterisk 20, they should work. A recent change (by me) has now created a conflict but it should be very easy to resolve.

asterisk-org-access-app bot requested a review from a team August 30, 2023 09:27

github-actions bot added the testing-in-progress label Aug 30, 2023

github-actions bot added the test-checks-passed label Aug 30, 2023

maximilianfridrich mentioned this pull request Aug 30, 2023

pjsip: Add tests for Advanced Codec Negotiation (ACN) asterisk/testsuite#20

Draft

jcolp marked this pull request as draft August 30, 2023 09:58

github-actions bot added test-gates-failed and removed testing-in-progress labels Aug 30, 2023

jcolp added the waiting-for-standard-release-development-cycle This change is pending the master branch being the next standard release development cycle. label Aug 31, 2023

gtjoseph force-pushed the master branch 4 times, most recently from c6b7d4b to 31abe63 Compare September 5, 2023 19:25

maximilianfridrich force-pushed the master-issue-223 branch from 709a3dc to b85addb Compare September 12, 2023 07:22

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Sep 12, 2023

btriller reviewed Nov 6, 2023

View reviewed changes

maximilianfridrich force-pushed the master-issue-223 branch from b85addb to 317a31e Compare November 15, 2023 08:45

maximilianfridrich force-pushed the master-issue-223 branch from 317a31e to eb5b500 Compare November 15, 2023 09:37

github-actions bot added pr-submit-testing-in-progress pr-submit-tests-failed and removed pr-submit-tests-failed pr-submit-testing-in-progress labels Nov 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core,app_dial,pjsip: Implement Advanced Codec Negotiation (ACN) #285

core,app_dial,pjsip: Implement Advanced Codec Negotiation (ACN) #285

maximilianfridrich commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

jcolp commented Aug 30, 2023 •

edited

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

jcolp commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

gtjoseph commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023 •

edited

maximilianfridrich commented Aug 31, 2023

InterLinked1 commented Aug 31, 2023

jcolp commented Aug 31, 2023

btriller commented Sep 11, 2023

maximilianfridrich commented Sep 12, 2023 •

edited

btriller commented Sep 22, 2023

maximilianfridrich commented Sep 22, 2023 •

edited

btriller Nov 6, 2023

maximilianfridrich Nov 15, 2023

btriller Nov 15, 2023

maximilianfridrich Nov 15, 2023

btriller commented Nov 8, 2023

btriller commented Nov 9, 2023

maximilianfridrich commented Nov 15, 2023

SirDMZ commented Apr 9, 2024

maximilianfridrich commented Apr 9, 2024 •

edited

core,app_dial,pjsip: Implement Advanced Codec Negotiation (ACN) #285

Are you sure you want to change the base?

core,app_dial,pjsip: Implement Advanced Codec Negotiation (ACN) #285

Conversation

maximilianfridrich commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

jcolp commented Aug 30, 2023 • edited

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

jcolp commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

jcolp commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

gtjoseph commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023

maximilianfridrich commented Aug 30, 2023 • edited

maximilianfridrich commented Aug 31, 2023

InterLinked1 commented Aug 31, 2023

jcolp commented Aug 31, 2023

btriller commented Sep 11, 2023

maximilianfridrich commented Sep 12, 2023 • edited

btriller commented Sep 22, 2023

maximilianfridrich commented Sep 22, 2023 • edited

btriller Nov 6, 2023

Choose a reason for hiding this comment

maximilianfridrich Nov 15, 2023

Choose a reason for hiding this comment

btriller Nov 15, 2023

Choose a reason for hiding this comment

maximilianfridrich Nov 15, 2023

Choose a reason for hiding this comment

btriller commented Nov 8, 2023

btriller commented Nov 9, 2023

maximilianfridrich commented Nov 15, 2023

SirDMZ commented Apr 9, 2024

maximilianfridrich commented Apr 9, 2024 • edited

jcolp commented Aug 30, 2023 •

edited

maximilianfridrich commented Aug 30, 2023 •

edited

maximilianfridrich commented Sep 12, 2023 •

edited

maximilianfridrich commented Sep 22, 2023 •

edited

maximilianfridrich commented Apr 9, 2024 •

edited