[Asterisk-code-review] Update support for SILK format. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: Update support for SILK format.
......................................................................


Patch Set 3: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/3136/3/res/res_format_attr_silk.c
File res/res_format_attr_silk.c:

PS3, Line 61: static struct silk_attr *get_default_silk_attr(struct ast_format *format)
            : {
            : 	struct silk_attr *attr;
            : 
            : 	attr = ast_malloc(sizeof(*attr));
            : 	if (!attr) {
            : 		return NULL;
            : 	}
            : 
            : 	attr_init(attr);
            : 
            : 	return attr;
            : }
This be dead code.  See dead code below where called.


PS3, Line 106: 	if (!attr) {
             : 		attr = get_default_silk_attr(cloned);
             : 		if (!attr) {
             : 			ao2_ref(cloned, -1);
             : 			return NULL;
             : 		}
             : 		ast_format_set_attribute_data(cloned, attr);
             : 	}
This be dead code.  You cannot successfully clone a silk format without the attr being non-NULL.


PS3, Line 174: 	/* Take the lowest max bitrate */
             : 	if (!attr1 || !attr2) {
             : 		attr_res->maxbitrate = 0;
             : 	} else {
             : 		attr_res->maxbitrate = MIN(attr1->maxbitrate, attr2->maxbitrate);
             : 	}
             : 
             : 	/* Only do dtx if both sides want it. DTX is a trade off between
             : 	 * computational complexity and bandwidth. */
             : 	if (!attr1 || !attr2) {
             : 		attr_res->dtx = 0;
             : 	} else {
             : 		attr_res->dtx = attr1->dtx && attr2->dtx ? 1 : 0;
             : 	}
             : 
             : 	/* Only do FEC if both sides want it.  If a peer specifically requests not
             : 	 * to receive with FEC, it may be a waste of bandwidth. */
             : 	if (!attr1 || !attr2) {
             : 		attr_res->fec = 0;
             : 	} else {
             : 		attr_res->fec = attr1->fec && attr2->fec ? 1 : 0;
             : 	}
             : 
             : 	/* Use the maximum packetloss percentage between the two attributes. This affects how
             : 	 * much redundancy is used in the FEC. */
             : 	if (!attr1 || !attr2) {
             : 		attr_res->packetloss_percentage = 0;
             : 	} else {
             : 		attr_res->packetloss_percentage = MAX(attr1->packetloss_percentage, attr2->packetloss_percentage);
             : 	}
if (!attr1 || !attr2) {
   attr_init(attr_res);
} else {
  The original code block that set the attributes.
}


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

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