[Asterisk-code-review] bridge: suppress initial connected line update on attended t... (asterisk[13])

[Richard Mudgett]

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

Change subject: bridge: suppress initial connected line update on attended transfer
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/9558/2//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/9558/2//COMMIT_MSG@21
PS2, Line 21: This patch suppress the connected line update on attended transfer
s/suppress/suppresses/


https://gerrit.asterisk.org/#/c/9558/2//COMMIT_MSG@23
PS2, Line 23: And also suppress the connected line update on "DTMF Swap"
            : between the target and the transferee.
Suppressing the connected line updates when Alice swaps between Bob and Charlie is wrong.  Alice must get the updates when switching between Bob and Charlie.  As a consequence Bob and Charlie must also get updates even if they *seem* redundant.  Any connected line interception routines you setup could change them however you want including to what was sent earlier to the channel driver.  Truly redundant updates are blocked before being passed to the channel driver.

I was pointing out in my earlier comment that when Alice switches between Bob and Charlie your lie to Charlie would get erased.  Your lie to Charlie can *only* be done for the *initial* connection between Alice and Charlie.  Any subsequent connected line updates cannot be suppressed.


You asked why there is a difference between blind transfers and attended transfers.  In the blind transfer case Alice tells Bob to call Charlie directly.  With a blind transfer Alice has nothing to do with actually calling Charlie so your dialplan works as you want.  In the attended transfer case Alice calls Charlie then transfers Charlie to Bob.  When Alice completes transferring Charlie to Bob a connected line update *must* be generated because Charlie was talking to Alice and is now talking with Bob.  Your dialplan does not work as you want in this case because it cannot handle *any* connected line updates.  Your dialplan is fragile.


https://gerrit.asterisk.org/#/c/9558/2/main/bridge_basic.c
File main/bridge_basic.c:

https://gerrit.asterisk.org/#/c/9558/2/main/bridge_basic.c@3433
PS2, Line 3433: 	/* We increase the refcount of the transfer target because ast_bridge_impart() will
              : 	 * steal the reference we already have. We need to keep a reference, so the only
              : 	 * choice is to give it a bump
              : 	 */
              : 	ast_channel_ref(props->transfer_target);
              : 	if (ast_bridge_impart(props->target_bridge, props->transfer_target, NULL, NULL,
              : 		AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
              : 		ast_log(LOG_ERROR, "Channel %s: Unable to place transfer target into bridge.\n",
              : 			ast_channel_name(bridge_channel->chan));
              : 		stream_failsound(props->transferer);
              : 		ast_bridge_channel_write_unhold(bridge_channel);
              : 		ast_hangup(props->transfer_target);
              : 		props->transfer_target = NULL;
              : 		attended_transfer_properties_shutdown(props);
              : 		return 0;
              : 	}
You need to redo this patch to *only* suppress the *initial* connected line update.  To suppress that connected line update we need to get Alice into the target bridge before the local channel connected to Charlie joins the target bridge with the AST_BRIDGE_IMPART_INHIBIT_JOIN_COLP flag.

* This bridge impart needs to be moved to attended_transfer_monitor_thread().
* The impart needs to be done after Alice is moved to the target bridge with bridge_move() in attended_transfer_monitor_thread().
* The impart needs to be done with the AST_BRIDGE_IMPART_INHIBIT_JOIN_COLP flag added.
* bridge_move() needs to be modified to not call bridge_do_move if the passed in channel is already in the destination bridge.
* Some off-nominal code needs to be fixed-up because of the relocated code.



-- 
To view, visit https://gerrit.asterisk.org/9558
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I346652661949b6611c23e431ede0dbea1be3017a
Gerrit-Change-Number: 9558
Gerrit-PatchSet: 2
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 21:10:09 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180801/04352c5f/attachment-0001.html>