[Asterisk-code-review] (WIP) sdp: Add support for setting connection address and cl... (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change. ( https://gerrit.asterisk.org/5350 )

Change subject: (WIP) sdp: Add support for setting connection address and clean up state.
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

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

Line 370: 			if (type_local == AST_MEDIA_TYPE_AUDIO || type_joint == AST_MEDIA_TYPE_VIDEO) {
This line is a bit weird. We know that type_local and type_joint are the same type, so the logic here is valid. But it seems strange that you  use type_local in one comparison and type_joint in a different one.


PS1, Line 368: 		/* If we can reuse an existing media stream then do so */
             : 		if (type_local == type_joint) {
             : 			if (type_local == AST_MEDIA_TYPE_AUDIO || type_joint == AST_MEDIA_TYPE_VIDEO) {
             : 				stream_state_local->instance = ao2_bump(stream_state_joint->instance);
             : 				continue;
             : 			}
             : 		}
             : 
             : 		if (type_local == AST_MEDIA_TYPE_AUDIO || type_local == AST_MEDIA_TYPE_VIDEO) {
             : 			/* We need to create a new RTP instance */
             : 			stream_state_local->instance = create_rtp(sdp_state->options, type_local);
             : 			if (!stream_state_local->instance) {
             : 				return -1;
             : 			}
             : 		}
             : 	}
I'm having a hard time understanding what this function is intending to do. On the surface, it's setting up local stream state RTP instances based on joint stream state RTP instances. But my questions are:

1) Where did the joint stream state RTP instances get created?
2) Why would we have created joint stream state RTP instances prior to local stream state RTP instances?
3) Depending on the envisioned way that the joint stream state RTP instances were created, this algorithm could result in excess RTP instances being created. Consider the case that the local stream state topology has one audio stream and one video stream. But the joint stream state topology has one video stream and one audio stream. We should be able to use the RTP instances from the joint stream state directly, but this algorithm would result in two new RTP instances being created instead.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7e5307239a9534420732de11c451a2705b6b681
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes