[Asterisk-code-review] rtp: Only accept the first payload for a format in SDP. (asterisk[14])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: rtp: Only accept the first payload for a format in SDP.
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/3894/3//COMMIT_MSG
Commit Message:

Line 17: This change makes it so that one a format has a payload
s/one/once/


https://gerrit.asterisk.org/#/c/3894/3/main/rtp_engine.c
File main/rtp_engine.c:

PS3, Line 786: 
             : 
This still needs to be part of the decision tree because current does not match to_match in any way.


Line 756:  * \brief Determine if a payload type is already present in mappings
Determine if a type of payload is already present in mappings.


PS3, Line 775: 		if (!current) {
             : 			continue;
             : 		} else if (current->asterisk_format && to_match->asterisk_format) {
There is no need to make this an else clause because the previous if can never fall through.

Should make the code like this:
if (!current) {
   continue;
}
if (current == to_match) {
   return 1;
}
if (current->payload == to_match->payload)
  continue;
}
if (current->asterisk_format && to_match->asterisk_format) {
...

ast_rtp_codecs_payloads_set_m_type() could pass in a to_match object that is literally already in the vector and not a different object that matches.


Line 778: 			if (ast_format_cmp(current->format, to_match->format) == AST_FORMAT_CMP_NOT_EQUAL) {
This patch is only going to work if the passed in formats don't compare the attributes.  i.e., only go as far in ast_format_cmp() as the format->codec pointer compare.  Otherwise, attributes make the formats compare as not equal.

Need an ast_format_cmp() that only compares as deep as the format->codec pointer and not go down to the format->interface->format_cmp() level.  Suggested name:
enum ast_format_cmp_res ast_format_cmp_codec(const struct ast_format *format1, const struct ast_format *format2);

Maybe we would only care if this were comparing video codecs?


Line 781: 				return 0;
This needs to be a continue in case there is a mapping with a higher payload value.  In fact this test should be done after the current == to_match test above.


-- 
To view, visit https://gerrit.asterisk.org/3894
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e8150860a3518cab36d00b1fab50f9352b64e60
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes