[Asterisk-code-review] Add primitive SFU support to bridge softmix. (asterisk[master])

[Richard Mudgett]

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

Change subject: Add primitive SFU support to bridge_softmix.
......................................................................


Patch Set 4: Code-Review-1

(17 comments)

https://gerrit.asterisk.org/#/c/5643/4/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:

Line 413:  * \brief Determine if a stream is a video destination stream.
Cut-n-paste.  Doesn't match the function.


PS4, Line 504: 		/* The +3 is for the two underscore separators and null terminator */
             : 		stream_clone_name_len = SOFTBRIDGE_VIDEO_DEST_LEN + strlen(channel_name) + strlen(ast_stream_get_name(stream)) + 4;
+3/+4 mismatch


Line 514: 		ast_stream_topology_append_stream(dest, stream_clone);
stream_clone is leaked if retval < 0


Line 532: 		ast_stream_topology_append_stream(dest, clone);
clone is leaked if retval < 0


Line 586: 	joiner_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(joiner->chan));
Not checked for failure (joiner_topology == NULL)


PS4, Line 589: 		ast_stream_topology_free(joiner_video);
             : 		ast_stream_topology_free(existing_video);
             : 		return;
joiner_topology leaked


Line 593: 	ast_channel_request_stream_topology_change(joiner->chan, joiner_topology, NULL);
joiner_topology leaked after calling as I don't see it used anymore.


Line 601: 		participant_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(joiner->chan));
participant_topology not checked for failure


PS4, Line 603: 			ast_stream_topology_free(joiner_video);
             : 			ast_stream_topology_free(existing_video);
             : 			return;
participant_topology is leaked


Line 607: 		ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);
participant_topology is leaked


Line 702: 		ast_stream_topology_append_stream(dest, stream_clone);
stream_clone leaked if retval < 0


Line 727: 		ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);
participant_topology leaked


Line 731: 	ast_channel_request_stream_topology_change(leaver->chan, leaver_topology, NULL);
leaver_topology leaked


Line 1688: 	AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {
Can this loop be extracted to unit test it?  I worry that when the maps get expanded because the bridge mapping is larger that the gaps in the to/from bridge vectors will be filled with 0 instead of -1.

I actually have the same concern with ast_stream_topology_map().


Line 1781: 		ast_stream_topology_append_stream(topology, stream);
stream leaked if retval < 0


Line 1810: 	params_caps = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
params_caps not NULL checked for failure


https://gerrit.asterisk.org/#/c/5643/4/main/bridge_channel.c
File main/bridge_channel.c:

PS4, Line 2477: 		case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:
              : 			/*
              : 			 * If a stream topology has changed then the bridge_channel's
              : 			 * media mapping needs to be updated.
              : 			 */
              : 			if (bridge_channel->bridge->technology->stream_topology_changed) {
              : 				bridge_channel->bridge->technology->stream_topology_changed(
              : 					bridge_channel->bridge, bridge_channel);
              : 			}
              : 			break;
If the callback is not supplied maybe should just call ast_bridge_channel_stream_map() as the default?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c44a829cc63acf8b596a337b2dc3c13898a6c4d
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes