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

[maximilianfridrich]

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]

testsuite-test-pr: 20
cherry-pick-to: none

[jcolp]

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

[maximilianfridrich]

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]

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]

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]

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.

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

[maximilianfridrich]

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]

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

[jcolp]

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]

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]

I've converted it to a draft.

[maximilianfridrich]

1. Default behavior with this change should match previous versions
2. 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]

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

[maximilianfridrich]

@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]

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]

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]

@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]

I've added it.

[github-actions]

REMINDER: If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process.

If you don't want it cherry-picked, please add a comment with cherry-pick-to: none so we don't keep asking.

If, after adding "cherry-pick-to" comments, you change your mind, please edit the comment to DELETE the header lines and add cherry-pick-to: none.

The currently active branches are now 20, 21, 22 and master.

[maximilianfridrich]

cherry-pick-to: none

[github-actions]

REMINDER: If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process.

If you don't want it cherry-picked, please add a comment with cherry-pick-to: none so we don't keep asking.

If, after adding "cherry-pick-to" comments, you change your mind, please edit the comment to DELETE the header lines and add cherry-pick-to: none.

The currently active branches are now 20, 21, 22 and master.

[github-actions]

REMINDER: If this PR applies to other branches, please add a comment with the appropriate cherry-pick-to headers as per the Create a Pull Request process.

If you don't want it cherry-picked, please add a comment with so we don't keep asking.

If, after adding cherry-pick-to comments, you change your mind, please edit the comment to DELETE the header lines and add .

The currently active branches are now 20, 21, 22 and master.

[khoegh]

I've had 7 crashes in as many weeks, handling several hundred thousand calls pr day.
180 is always received less than a ms after the 183 which makes asterisk crash.
There are small variations, where the crash is.
chan_pjsip.c:1611 is in common, which is:
ast_channel_tech(in)->indicate(in, AST_CONTROL_PROGRESS, top, 0);
guarded by ast_channel_lock(in);

When 180/SDP is received, there is no channel lock, and it is using the "old"
ast_indicate(in, AST_CONTROL_RINGING);

I had a crash today. I have lots of calls between the same endpoints which look similar without crashes. I think it is a timing issue. The called endpoint responded 183 followed by 180 after 0.000358 second. (Both had SDP) Unfortunately I'm unable to fix the code. Hope the following can help anyone interested.

#0 ast_stream_topology_get_first_stream_by_type (type=AST_MEDIA_TYPE_AUDIO, topology=0x7f9038a9d5a8) at stream.c:976 976 if (stream->type == type [Current thread is 1 (Thread 0x7f8fee524700 (LWP 3461580))] (gdb) p stream $1 = (struct ast_stream *) 0x7f9785dda96a (gdb) p stream->type Cannot access memory at address 0x7f9785dda96a

(gdb) bt #0 ast_stream_topology_get_first_stream_by_type (type=AST_MEDIA_TYPE_AUDIO, topology=0x7f9038a9d5a8) at stream.c:976 #1 ast_stream_topology_create_resolved (pending_topology=pending_topology@entry=0x7f907cc439f8, configured_topology=0x7f9038a9d5a8, prefs=prefs@entry=0x157b30c, error_message=error_message@entry=0x7f8fee51eca0) at stream.c:1046 #2 0x00007f90b0298c69 in chan_pjsip_indicate (ast=0x7f903a1ee050, condition=14, data=0x7f907cc439f8, datalen=0) at chan_pjsip.c:1836 #3 0x00007f9104a6d138 in wait_for_answer (in=in@entry=0x7f903a1ee050, out_chans=out_chans@entry=0x7f8fee51fde0, to_answer=to_answer@entry=0x7f8fee51fda0, to_progress=to_progress@entry=0x7f8fee51fda4, peerflags=peerflags@entry=0x7f8fee520938, opt_args=opt_args@entry=0x7f8fee520080, pa=0x7f8fee5202e0, num_in=0x7f8fee51fe00, result=0x7f8fee51fdac, dtmf_progress=, mf_progress=, mf_wink=, sf_progress=, sf_wink=, hearpulsing=0, ignore_cc=1, forced_clid=0x7f8fee51fe50, stored_clid=0x7f8fee51fea0, config=) at app_dial.c:1611

[khoegh]

I've used the following patch for a couple of weeks (~5M calls) without crashes and haven't noticed any side effects.

--- a/apps/app_dial.c   2024-12-02 15:34:43.486312781 +0100
+++ b/apps/app_dial.c   2025-02-12 16:03:57.472812221 +0100
@@ -1574,7 +1574,11 @@
                                                        ast_channel_early_bridge(in, c);
                                                }
                                                if (!(pa->sentringing) && !ast_test_flag64(outgoing, OPT_MUSICBACK) && ast_strlen_zero(opt_args[OPT_ARG_RINGBACK])) {
-                                                       ast_indicate(in, AST_CONTROL_RINGING);
+                                                       struct ast_stream_topology *top = ao2_bump(ast_channel_get_stream_topology(c));
+                                                       ast_channel_lock(in);
+                                                       ast_channel_tech(in)->indicate(in, AST_CONTROL_RINGING, top, 0);
+                                                       ast_channel_unlock(in);
+                                                       ao2_ref(top, -1);
                                                        pa->sentringing++;
                                                }
                                                if (!sent_ring) {

I've had 7 crashes in as many weeks, handling several hundred thousand calls pr day. 180 is always received less than a ms after the 183 which makes asterisk crash. There are small variations, where the crash is. chan_pjsip.c:1611 is in common, which is: ast_channel_tech(in)->indicate(in, AST_CONTROL_PROGRESS, top, 0); guarded by ast_channel_lock(in);

When 180/SDP is received, there is no channel lock, and it is using the "old" ast_indicate(in, AST_CONTROL_RINGING);

I had a crash today. I have lots of calls between the same endpoints which look similar without crashes. I think it is a timing issue. The called endpoint responded 183 followed by 180 after 0.000358 second. (Both had SDP) Unfortunately I'm unable to fix the code. Hope the following can help anyone interested.
#0 ast_stream_topology_get_first_stream_by_type (type=AST_MEDIA_TYPE_AUDIO, topology=0x7f9038a9d5a8) at stream.c:976 976 if (stream->type == type [Current thread is 1 (Thread 0x7f8fee524700 (LWP 3461580))] (gdb) p stream $1 = (struct ast_stream *) 0x7f9785dda96a (gdb) p stream->type Cannot access memory at address 0x7f9785dda96a
(gdb) bt #0 ast_stream_topology_get_first_stream_by_type (type=AST_MEDIA_TYPE_AUDIO, topology=0x7f9038a9d5a8) at stream.c:976 #1 ast_stream_topology_create_resolved (pending_topology=pending_topology@entry=0x7f907cc439f8, configured_topology=0x7f9038a9d5a8, prefs=prefs@entry=0x157b30c, error_message=error_message@entry=0x7f8fee51eca0) at stream.c:1046 #2 0x00007f90b0298c69 in chan_pjsip_indicate (ast=0x7f903a1ee050, condition=14, data=0x7f907cc439f8, datalen=0) at chan_pjsip.c:1836 #3 0x00007f9104a6d138 in wait_for_answer (in=in@entry=0x7f903a1ee050, out_chans=out_chans@entry=0x7f8fee51fde0, to_answer=to_answer@entry=0x7f8fee51fda0, to_progress=to_progress@entry=0x7f8fee51fda4, peerflags=peerflags@entry=0x7f8fee520938, opt_args=opt_args@entry=0x7f8fee520080, pa=0x7f8fee5202e0, num_in=0x7f8fee51fe00, result=0x7f8fee51fdac, dtmf_progress=, mf_progress=, mf_wink=, sf_progress=, sf_wink=, hearpulsing=0, ignore_cc=1, forced_clid=0x7f8fee51fe50, stored_clid=0x7f8fee51fea0, config=) at app_dial.c:1611