rtp_engine: add support for multirate 2833 DRAFT#1
Conversation
|
cherry-pick-to: none |
jcolp
left a comment
jcolp
left a comment
There was a problem hiding this comment.
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.
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
from
b74fbb4 to
df8b425
Compare
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
from
df8b425 to
8bbd10c
Compare
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
2 times, most recently
from
a942c8e to
29ac748
Compare
res/res_rtp_asterisk.c
Outdated
| 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) { |
There was a problem hiding this comment.
This review needs a full review of spacing, if (payload_format) {
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
from
29ac748 to
146e916
Compare
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
6 times, most recently
from
ce7eec7 to
563a625
Compare
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
from
563a625 to
494e3f2
Compare
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
2 times, most recently
from
70a38c2 to
5166a55
Compare
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
from
5166a55 to
ff866a0
Compare
res/res_pjsip_sdp_rtp.c
Outdated
| if (AST_VECTOR_INIT(&sampleRates, 1)) { | ||
| ast_log(LOG_ERROR, "Unable to allocate dtmf payload types.\n"); | ||
| buildSampleRates = 0; | ||
| }; |
res/res_pjsip_sdp_rtp.c
Outdated
|
|
||
| /* 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"); |
There was a problem hiding this comment.
From a user perspective this isn't really informational
res/res_pjsip_sdp_rtp.c
Outdated
| 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; |
There was a problem hiding this comment.
build_dtmf_sample_rates to be more accurate and follow existing variable naming
res/res_pjsip_sdp_rtp.c
Outdated
| if ((attr = generate_rtpmap_attr(session, media, pool, rtp_code, 0, NULL, index))) { | ||
| media->attr[media->attr_count++] = attr; | ||
| } | ||
| } |
| * | ||
| * This looks for the numerical payload for a DTMF type with a sample rate of 8kHz in the codecs structure. | ||
| * | ||
| * \since 21.0.0 |
There was a problem hiding this comment.
Version on all of these will need to be updated whenever this goes in.
| 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++) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think we are safe, elements only get replaced. This is how other functions walk this vector.
main/rtp_engine.c
Outdated
| */ | ||
| 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 |
| 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); |
There was a problem hiding this comment.
Is there a reason 48k needs to be right here, or does order not matter?
There was a problem hiding this comment.
These are the two I think are most common, but I could be biased based on how I was testing.
| 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; |
There was a problem hiding this comment.
Does this need to be initialized to anything?
There was a problem hiding this comment.
I don't think so, this function is a copy of generate_rtpmap_attr but with the sample_rate variable.
res/res_pjsip_sdp_rtp.c
Outdated
|
|
||
| /* Keep track of the sample rates for offered codecs so we can build matching | ||
| 2833 payload offers. */ | ||
| AST_VECTOR(, int) sampleRates; |
mbradeen
force-pushed
the
rtp-engine-2833-multirate
branch
from
ff866a0 to
adfa991
Compare
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
3 participants
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