Repository navigation
Conversation
Support NG codec dictionaries, Kamailio's top-level alias, and rtpp-flags. Preserve existing telephone-event forwarding and reject invalid payload preferences before commands execute. Port integration coverage and parser fixes onto current upstream master.
…dtmf-payload-master-upstream # Conflicts: # t/Makefile
rfuchs
left a comment
rfuchs
left a comment
There was a problem hiding this comment.
As usual with generated code, lots of things that can be improved...
In general, it would be nicer if the payload remapping can be done within the existing code, instead of requiring an extra pass over the final list of codecs. But OK, it's an optional feature, so not a big deal.
Even more in general, it would be even nicer if this could be done for any RTP payload type, instead of being specific to telephone-event. There might be cases where e.g. a user might require Opus to be on 98 instead of 96, or w/e.
| } | ||
| const char *call_ng_codec_flags(const ng_parser_t *parser, str *key, parser_arg value, helper_arg arg) { | ||
| sdp_ng_flags *out = arg.flags; | ||
| if (!str_cmp(key, "telephone-event-payload")) { |
There was a problem hiding this comment.
This should go into the csh_lookup switch below, together with the other flags
| #ifndef WITH_TRANSCODING | ||
| return "telephone-event-payload requires transcoding support"; | ||
| #else | ||
| if (out->opmode != OP_OFFER) |
There was a problem hiding this comment.
subscribe-request and create are also relevant
| if (parser->is_int(value)) | ||
| pt = parser->get_int(value); | ||
| else { | ||
| str number; | ||
| if (parser->get_str(value, &number)) { | ||
| for (size_t i = 0; i < number.len; i++) { | ||
| if (number.s[i] < '0' || number.s[i] > '9') | ||
| return "telephone-event-payload must be an integer from 96 to 127"; | ||
| } | ||
| pt = str_to_ll(&number, -1); | ||
| } | ||
| } |
There was a problem hiding this comment.
We have get_int_str which already does this
| * @param dict - root dict to store encoded flags | ||
| */ | ||
| void parse_rtpp_flags(const str * rtpp_flags, sdp_ng_flags *out) | ||
| const char *parse_rtpp_flags(const str * rtpp_flags, sdp_ng_flags *out) |
There was a problem hiding this comment.
Ideally the change of return value for this function to support an error message should go into a separate commit, as it's not directly related to the new feature
| /* An empty payload preference is an invalid value, not a boolean flag. */ | ||
| if (!val.len && !str_eq(&key, "codec-telephone-event-payload")) |
There was a problem hiding this comment.
This exception seems out of place. The handler function for the option key should be able to handle it without requiring this here
| `codec` contains a dictionary controlling various aspects of codecs (or RTP payload types). | ||
|
|
||
| These options can also be put into the `flags` list using a prefix of `codec-`. For example, | ||
| Except for `telephone-event-payload`, these options can also be put into the `flags` list using a prefix of `codec-`. For example, |
There was a problem hiding this comment.
The patch explicitly supports "codec-telephone-event-payload= as a top-level option key, so this documentation is wrong
| out->dtls_passive = dtls_passive_def; | ||
| out->el_option = rtpe_config.endpoint_learning; | ||
| out->tos = 256; | ||
| out->telephone_event_payload = -1; |
There was a problem hiding this comment.
Since zero is an invalid value for this, it would be simpler to just leave it caught in the default zero-init
| if (payload_type < 96 || payload_type > 127) | ||
| return; |
There was a problem hiding this comment.
FTR, some clients (WebRTC...) support and do use payload type numbers < 96 (those which aren't mapped to a fixed codec by an RFC) for custom payload types. So I think it would be prudent to allow this use if it's really wanted.
This change adds an optional preference for the outgoing telephone-event RTP payload type in offers. It allows endpoints to negotiate different telephone-event payload types while preserving DTMF forwarding.
The option can be supplied through the NG codec dictionary, the top-level codec-telephone-event-payload alias used by Kamailio, or rtpp-flags. It accepts payload types from 96 to 127 and rejects invalid values before executing the command.
Testing