[Asterisk-code-review] bridge simple: Added support for streams (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/5528 )

Change subject: bridge_simple: Added support for streams
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/5528/2/bridges/bridge_simple.c
File bridges/bridge_simple.c:

PS2, Line 94: 	if (bridge_channel && ast_channel_get_stream_topology_change_source(
            : 		    bridge_channel->chan) == &simple_bridge) {
            : 		return;
            : 	}
In what case will there not be a bridge_channel? That should be documented


https://gerrit.asterisk.org/#/c/5528/2/include/asterisk/bridge_channel.h
File include/asterisk/bridge_channel.h:

Line 714:  * \brief Maps a channel's stream topology to and from the bridge
I think further details on what this means is needed


https://gerrit.asterisk.org/#/c/5528/2/include/asterisk/bridge_technology.h
File include/asterisk/bridge_technology.h:

Line 162: 	 * \brief Callback for when a stream topology changes on the channel
Is this called when we receive a REQUEST to change the topology? That's different than when it actually does change. Further information about what the behavior should be is also needed by the implementation.


https://gerrit.asterisk.org/#/c/5528/2/main/bridge.c
File main/bridge.c:

Line 756: 	AST_VECTOR_INIT(&bridge->media_types, 7);
Magic number 7.


https://gerrit.asterisk.org/#/c/5528/2/main/bridge_channel.c
File main/bridge_channel.c:

Line 2462: 		case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:
What about AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE?

This can happen if something external requests that the topology change (for example, they request that we add a video stream). That should also be handled in here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1eb5987921dd80c3cdcf52accc136393ca2d4163
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes