[Asterisk-code-review] bridge native rtp: Keep references to rtp instances on hook... (asterisk[13])

[Richard Mudgett]

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

Change subject: bridge_native_rtp:  Keep references to rtp instances on hook data
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

The whole point of getting those RTP instance refs is so they are still around and accessible by native_rtp_bridge_stop() to clear the instance->local_bridge() pointer safely on both channels.  native_rtp_bridge_stop() should not need to call native_rtp_bridge_get() for the instance ponters to unlink the bridged instances.

Placing the rtp instance refs in the bridge_channel tech_pvt struct used for the framehook control will not hold the refs for the needed lifetime since native_rtp_bridge_stop() is called after native_rtp_bridge_framehook_detach() destroys it.

There is a bridge->tech_pvt pointer available that can be used to hold a vector of rtp instance pointers.  When native_rtp_bridge_start() sets the rtp instance bridged pointers in the rtp instances and in the vector.  native_rtp_bridge_stop() can unlink those rtp instance bridged pointers from the saved vector.

This was discussed on IRC.

https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c
File bridges/bridge_native_rtp.c:

https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c@170
PS1, Line 170: 	if (bc0 == bc1) {
             : 		ast_debug(2, "Bridge '%s' doesn't start when both channels are '%s'\n", bridge->uniqueid,
             : 			ast_channel_name(bc0->chan));
This test is false if there are two or more channels in the bridge so the debug is not accurate.


https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c@195
PS1, Line 195: 		ast_debug(2, "Locally RTP bridged '%s' and '%s' in stack\n",
             : 			ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
This is redundant with the verbose message.


https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c@213
PS1, Line 213: 			ast_debug(2, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
             : 				ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
redundant


https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c@525
PS1, Line 525: 	// get_rtp_info bumps the instance refcount for us
C++ comments


https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c@576
PS1, Line 576:  * \brief For debugging bridge lifecycle only
wth?


https://gerrit.asterisk.org/#/c/5854/1/bridges/bridge_native_rtp.c@596
PS1, Line 596:  * \brief For debugging bridge lifecycle only
wth?



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a
Gerrit-Change-Number: 5854
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 19 Jun 2017 15:54:50 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170619/0939a25c/attachment.html>