rtp_engine: add support for multirate 2833 DRAFT #1

mbradeen · 2023-11-28T16:59:58Z

DRAFT PR - demonstrate a method for handling non 8K 2833 digit sdp offers.

Changes to the engine itself limited to adding the 48/24K types.

res_pjsip_sdp_rtp is changed to note the number of incoming codecs and add an associated offer

mbradeen · 2023-11-28T17:00:43Z

cherry-pick-to: none

jcolp

I don't think the actual RTP traffic will flow as expected here. How does it know which negotiated AST_RTP_DTMF to use at a given time? As well, I believe res_rtp_asterisk is hardcoded to assume 8000Hz for DTMF for sending, specifically for timestamps. I think it needs to use the negotiated sample rate instead.

main/rtp_engine.c

res/res_pjsip_sdp_rtp.c

include/asterisk/rtp_engine.h

main/rtp_engine.c

res/res_pjsip_sdp_rtp.c

res/res_rtp_asterisk.c

include/asterisk/rtp_engine.h

main/rtp_engine.c

res/res_pjsip_sdp_rtp.c

res/res_rtp_asterisk.c

main/rtp_engine.c

include/asterisk/format.h

include/asterisk/rtp_engine.h

main/format.c

main/rtp_engine.c

res/res_pjsip_sdp_rtp.c

res/res_rtp_asterisk.c

jcolp · 2024-01-11T00:21:04Z

res/res_rtp_asterisk.c

+	if (rtp->lasttxformat == ast_format_none) {
+		/* No audio frames have been written yet so we have to lookup both the preferred payload type and bitrate. */
+		payload_format = ast_rtp_codecs_get_preferred(ast_rtp_instance_get_codecs(instance));
+		if(payload_format) {


This review needs a full review of spacing, if (payload_format) {

res/res_rtp_asterisk.c

main/rtp_engine.c

jcolp

Minor comments.

include/asterisk/rtp_engine.h

main/rtp_engine.c

res/res_pjsip_sdp_rtp.c

include/asterisk/rtp_engine.h

res/res_pjsip_sdp_rtp.c

jcolp · 2024-02-01T10:12:42Z

res/res_pjsip_sdp_rtp.c

+	if (AST_VECTOR_INIT(&sampleRates, 1)) {
+		ast_log(LOG_ERROR, "Unable to allocate dtmf payload types.\n");
+		buildSampleRates = 0;
+	};


jcolp · 2024-02-01T10:12:59Z

res/res_pjsip_sdp_rtp.c


+	/* Init the sample rates before we start adding them. Assume we will have at least one. */
+	if (AST_VECTOR_INIT(&sampleRates, 1)) {
+		ast_log(LOG_ERROR, "Unable to allocate dtmf payload types.\n");


From a user perspective this isn't really informational

jcolp · 2024-02-01T10:13:49Z

res/res_pjsip_sdp_rtp.c

+	   2833 payload offers. */
+	AST_VECTOR(, int) sampleRates;
+	/* In case we can't init the sample rates, still try to do the rest. */
+	int buildSampleRates = 1;


build_dtmf_sample_rates to be more accurate and follow existing variable naming

jcolp · 2024-02-01T10:14:09Z

res/res_pjsip_sdp_rtp.c

+				if ((attr = generate_rtpmap_attr(session, media, pool, rtp_code, 0, NULL, index))) {
+					media->attr[media->attr_count++] = attr;
+				}
+			}


} else if {

bkford · 2024-02-05T19:18:18Z

include/asterisk/rtp_engine.h

+ *
+ * This looks for the numerical payload for a DTMF type with a sample rate of 8kHz in the codecs structure.
+ *
+ * \since 21.0.0


Version on all of these will need to be updated whenever this goes in.

bkford · 2024-02-05T19:36:00Z

main/rtp_engine.c

 	ast_rwlock_wrlock(&codecs->codecs_lock);
+
+	/* Go through the existing mapping to create an ignore list. */
+	for (i = 0; i < AST_VECTOR_SIZE(&codecs->payload_mapping_rx); i++) {


This is just me thinking, but could this ever be higher than AST_RTP_MAX_PT or does codecs->payload_mapping_rx also abide by that sizing limit?

I think we are safe, elements only get replaced. This is how other functions walk this vector.

bkford · 2024-02-05T19:37:31Z

main/rtp_engine.c

 		 */
 		rtp_codecs_payload_replace_rx(codecs, payload, new_type);
+	} else if ( payload > -1 && !explicit
+				/* We can either call this with the the full list or the current rx list. The former


double "the"

bkford · 2024-02-05T19:41:28Z

main/rtp_engine.c

 	set_next_mime_type(ast_format_g726_aal2, 0, "audio", "AAL2-G726-32", 8000);
+	/* we need all possible dtmf/bitrate combinations or ast_rtp_codecs_payloads_set_rtpmap_type_rate will not examine it */
 	set_next_mime_type(NULL, AST_RTP_DTMF, "audio", "telephone-event", 8000);
+	set_next_mime_type(NULL, AST_RTP_DTMF, "audio", "telephone-event", 48000);


Is there a reason 48k needs to be right here, or does order not matter?

These are the two I think are most common, but I could be biased based on how I was testing.

bkford · 2024-02-05T19:46:33Z

res/res_pjsip_sdp_rtp.c

+					      int rtp_code, int asterisk_format, struct ast_format *format, int code, int sample_rate)
+{
+#ifndef HAVE_PJSIP_ENDPOINT_COMPACT_FORM
+	extern pj_bool_t pjsip_use_compact_form;


Does this need to be initialized to anything?

I don't think so, this function is a copy of generate_rtpmap_attr but with the sample_rate variable.

bkford · 2024-02-05T19:48:28Z

res/res_pjsip_sdp_rtp.c

+
+	/* Keep track of the sample rates for offered codecs so we can build matching
+	   2833 payload offers. */
+	AST_VECTOR(, int) sampleRates;


sample_rates

DRAFT PR - demonstrate a method for handling non 8K 2833 digit sdp offers. Changes to the engine itself limited to adding the 48/24K types. res_pjsip_sdp_rtp is changed to note the number of incoming codecs and add an associated offer

mbradeen marked this pull request as draft November 28, 2023 17:00

jcolp suggested changes Nov 28, 2023

View reviewed changes

main/rtp_engine.c Show resolved Hide resolved

res/res_pjsip_sdp_rtp.c Outdated Show resolved Hide resolved

mbradeen force-pushed the rtp-engine-2833-multirate branch from b74fbb4 to df8b425 Compare December 11, 2023 19:37

jcolp suggested changes Dec 12, 2023

View reviewed changes

mbradeen force-pushed the rtp-engine-2833-multirate branch from df8b425 to 8bbd10c Compare January 8, 2024 18:57

jcolp suggested changes Jan 10, 2024

View reviewed changes

mbradeen force-pushed the rtp-engine-2833-multirate branch 2 times, most recently from a942c8e to 29ac748 Compare January 10, 2024 21:48

jcolp suggested changes Jan 11, 2024

View reviewed changes

mbradeen force-pushed the rtp-engine-2833-multirate branch from 29ac748 to 146e916 Compare January 12, 2024 00:37

jcolp suggested changes Jan 12, 2024

View reviewed changes

main/rtp_engine.c Outdated Show resolved Hide resolved

mbradeen force-pushed the rtp-engine-2833-multirate branch 6 times, most recently from ce7eec7 to 563a625 Compare January 18, 2024 01:08

mbradeen force-pushed the rtp-engine-2833-multirate branch from 563a625 to 494e3f2 Compare January 24, 2024 20:01

jcolp suggested changes Jan 29, 2024

View reviewed changes

mbradeen force-pushed the rtp-engine-2833-multirate branch 2 times, most recently from 70a38c2 to 5166a55 Compare January 29, 2024 18:44

jcolp suggested changes Jan 29, 2024

View reviewed changes

mbradeen force-pushed the rtp-engine-2833-multirate branch from 5166a55 to ff866a0 Compare January 30, 2024 17:48

jcolp suggested changes Feb 1, 2024

View reviewed changes

bkford suggested changes Feb 5, 2024

View reviewed changes

mbradeen force-pushed the rtp-engine-2833-multirate branch from ff866a0 to adfa991 Compare February 22, 2024 20:12

rtp_engine: add support for multirate 2833 DRAFT

1c35432

DRAFT PR - demonstrate a method for handling non 8K 2833 digit sdp offers. Changes to the engine itself limited to adding the 48/24K types. res_pjsip_sdp_rtp is changed to note the number of incoming codecs and add an associated offer

mbradeen force-pushed the rtp-engine-2833-multirate branch from adfa991 to 1c35432 Compare February 22, 2024 20:23

jcolp approved these changes Feb 23, 2024

View reviewed changes

rtp_engine: add support for multirate 2833 DRAFT #1

Are you sure you want to change the base?

rtp_engine: add support for multirate 2833 DRAFT #1

Uh oh!

Conversation

mbradeen commented Nov 28, 2023

Uh oh!

mbradeen commented Nov 28, 2023

Uh oh!

jcolp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jcolp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!