[Asterisk-code-review] sdp: Add support for T.38 (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5496 )

Change subject: sdp: Add support for T.38
......................................................................


Patch Set 1: Code-Review-1

(13 comments)

Code improvements.

https://gerrit.asterisk.org/#/c/5496/1/main/sdp.c
File main/sdp.c:

Line 708: 	if (ast_stream_get_type(stream) == AST_MEDIA_TYPE_AUDIO || ast_stream_get_type(stream) == AST_MEDIA_TYPE_VIDEO) {
Would be better if this else-if ladder were converted to a switch.  That way you don't need to call ast_stream_get_type() repeatedly.  An additional benefit is the switch could complain if a case is missing.


https://gerrit.asterisk.org/#/c/5496/1/main/sdp_state.c
File main/sdp_state.c:

Line 100: 	if (state_stream->type == AST_MEDIA_TYPE_AUDIO || state_stream->type == AST_MEDIA_TYPE_VIDEO) {
Use switch instead


Line 264: 		if (state_stream->type == AST_MEDIA_TYPE_AUDIO || state_stream->type == AST_MEDIA_TYPE_VIDEO) {
Use switch


PS1, Line 384: 	type = ast_stream_get_type(ast_stream_topology_get_stream(sdp_state->proposed_capabilities->topology,
             : 		stream_index));
             : 	if (type != AST_MEDIA_TYPE_AUDIO && type != AST_MEDIA_TYPE_VIDEO) {
             : 		return NULL;
             : 	}
Should this be an assert instead of a run time check?

The caller should already know if he is dealing with RTP vs UDPTL right?


PS1, Line 406: 	type = ast_stream_get_type(ast_stream_topology_get_stream(sdp_state->proposed_capabilities->topology,
             : 		stream_index));
             : 	if (type != AST_MEDIA_TYPE_IMAGE) {
             : 		return NULL;
             : 	}
Should this be an assert instead of a run time check?

The caller should already know if he is dealing with RTP vs UDPTL right?


PS1, Line 450: 	if (type == AST_MEDIA_TYPE_AUDIO || type == AST_MEDIA_TYPE_VIDEO) {
             : 		ast_rtp_instance_get_local_address(stream_state->instance, address);
             : 	} else if (type == AST_MEDIA_TYPE_IMAGE) {
             : 		ast_udptl_get_us(stream_state->udptl->instance, address);
             : 	} else {
             : 		return -1;
             : 	}
Use switch


PS1, Line 657: 			if (joint_state_stream->type == AST_MEDIA_TYPE_AUDIO || joint_state_stream->type == AST_MEDIA_TYPE_VIDEO) {
             : 				joint_state_stream->instance = ao2_bump(current_state_stream->instance);
             : 			} else if (joint_state_stream->type == AST_MEDIA_TYPE_IMAGE) {
use switch


PS1, Line 661: 				memcpy(&joint_state_stream->t38_local_params, &current_state_stream->t38_local_params,
             : 					sizeof(joint_state_stream->t38_local_params));
Rather than a memcpy:

joint_state_stream->t38_local_params = current_state_stream->t38_local_params;


Line 679: 			if (new_stream_type == AST_MEDIA_TYPE_AUDIO || new_stream_type == AST_MEDIA_TYPE_VIDEO) {
use switch


Line 882: 	RAII_VAR(struct ast_sockaddr *, addrs, NULL, ast_free);
This RAII_VAR can be avoided by inverting the ast_sockaddr_resolve() if test.


PS1, Line 986: 		if ((stream_type == AST_MEDIA_TYPE_AUDIO || stream_type == AST_MEDIA_TYPE_VIDEO) && state_stream->instance) {
             : 			update_rtp_after_merge(sdp_state, state_stream->instance, sdp_state->options,
             : 				remote_sdp, ast_sdp_get_m(remote_sdp, i));
             : 		} else if (stream_type == AST_MEDIA_TYPE_IMAGE) {
Use switch


Line 1150: 	memcpy(&stream_state->t38_local_params, params, sizeof(stream_state->t38_local_params));
stream_state->t38_local_params = *params;


Line 1506: 		if (type == AST_MEDIA_TYPE_AUDIO || type == AST_MEDIA_TYPE_VIDEO) {
use switch


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If28956762ccb8ead562ac6c03d162d3d6014f2c7
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes