[Asterisk-code-review] core_unreal: Fix T.38 faxing when using local channels. (asterisk[18])

[Kevin Harwell]

Kevin Harwell has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/15362 )

Change subject: core_unreal: Fix T.38 faxing when using local channels.
......................................................................

core_unreal: Fix T.38 faxing when using local channels.

After some changes to streams and topologies, receiving fax through
local channels stopped working. This change adds a stream topology with
a stream of type IMAGE to the local channel pair and allows fax to be
received.

ASTERISK-29035 #close

Change-Id: Id103cc5c9295295d8e68d5628e76220f8f17e9fb
---
M include/asterisk/core_unreal.h
M main/core_unreal.c
2 files changed, 90 insertions(+), 0 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, approved; Approved for Submit
  George Joseph: Verified



diff --git a/include/asterisk/core_unreal.h b/include/asterisk/core_unreal.h
index a28d39d..c6e770b 100644
--- a/include/asterisk/core_unreal.h
+++ b/include/asterisk/core_unreal.h
@@ -98,6 +98,8 @@
 	/*! Base name of the unreal channels.  exten at context or other name. */
 	char name[AST_MAX_EXTENSION + AST_MAX_CONTEXT + 2];
 	struct ast_stream_topology *reqtopology;    /*!< Requested stream topology */
+	struct ast_stream_topology *owner_old_topology;	/*!< Stored topology for owner side when we need to restore later (faxing) */
+	struct ast_stream_topology *chan_old_topology;	/*!< Stored topology for chan side when we need to restore later (faxing) */
 };
 
 #define AST_UNREAL_IS_OUTBOUND(a, b) ((a) == (b)->chan ? 1 : 0)
diff --git a/main/core_unreal.c b/main/core_unreal.c
index 16414e6..5adc90d 100644
--- a/main/core_unreal.c
+++ b/main/core_unreal.c
@@ -623,6 +623,9 @@
 {
 	struct ast_unreal_pvt *p = ast_channel_tech_pvt(ast);
 	int res = 0;
+	struct ast_channel *chan = NULL;
+	struct ast_channel *owner = NULL;
+	const struct ast_control_t38_parameters *parameters;
 
 	if (!p) {
 		return -1;
@@ -677,6 +680,89 @@
 			res = unreal_colp_stream_topology_request_change(p, ast, data);
 		}
 		break;
+	case AST_CONTROL_T38_PARAMETERS:
+		parameters = data;
+		if (parameters->request_response == AST_T38_NEGOTIATED) {
+			struct ast_stream *stream;
+			struct ast_stream_topology *new_topology;
+
+			stream = ast_stream_alloc("local_fax", AST_MEDIA_TYPE_IMAGE);
+			if (!stream) {
+				ast_log(LOG_ERROR, "Failed to allocate memory for stream.\n");
+				res = -1;
+				break;
+			}
+			new_topology = ast_stream_topology_alloc();
+			if (!new_topology) {
+				ast_log(LOG_ERROR, "Failed to allocate memory for stream topology.\n");
+				ast_free(stream);
+				res = -1;
+				break;
+			}
+			ast_stream_topology_append_stream(new_topology, stream);
+
+			/*
+			 * Lock both parts of the local channel so we can store their topologies and replace them with
+			 * one that has a stream with type IMAGE. We can just hold the reference on the unreal_pvt
+			 * structure and bump it, then steal the ref later when we are restoring the topology.
+			 *
+			 * We use ast_unreal_lock_all here because we don't know if the ;1 or ;2 side will get the
+			 * signaling and we need to be sure that the locking order is the same to prevent possible
+			 * deadlocks.
+			 */
+			ast_unreal_lock_all(p, &chan, &owner);
+
+			if (owner) {
+				p->owner_old_topology = ao2_bump(ast_channel_get_stream_topology(owner));
+				ast_channel_set_stream_topology(owner, new_topology);
+			}
+
+			if (chan) {
+				p->chan_old_topology = ao2_bump(ast_channel_get_stream_topology(chan));
+
+				/* Bump the ref for new_topology, since it will be used by both sides of the local channel */
+				ao2_ref(new_topology, +1);
+				ast_channel_set_stream_topology(chan, new_topology);
+			}
+
+			ao2_unlock(p);
+		} else if (parameters->request_response == AST_T38_TERMINATED) {
+			/*
+			 * Lock both parts of the local channel so we can restore their topologies to the original.
+			 * The topology should be on the unreal_pvt structure, with a ref that we can steal. Same
+			 * conditions as above.
+			 */
+			ast_unreal_lock_all(p, &chan, &owner);
+
+			if (owner) {
+				ast_channel_set_stream_topology(owner, p->owner_old_topology);
+				p->owner_old_topology = NULL;
+			}
+
+			if (chan) {
+				ast_channel_set_stream_topology(chan, p->chan_old_topology);
+				p->chan_old_topology = NULL;
+			}
+
+			ao2_unlock(p);
+		}
+
+		/*
+		 * We unlock ast_unreal_pvt in the above conditionals since there's no way to
+		 * tell if it's been unlocked already or not when we get to this point, but
+		 * if either of these are not NULL, we know that they are locked and need to
+		 * unlock them.
+		 */
+		if (owner) {
+			ast_channel_unlock(owner);
+			ast_channel_unref(owner);
+		}
+
+		if (chan) {
+			ast_channel_unlock(chan);
+			ast_channel_unref(chan);
+		}
+		/* Fall through for all T38 conditions */
 	default:
 		res = unreal_queue_indicate(p, ast, condition, data, datalen);
 		break;
@@ -1012,6 +1098,8 @@
 	doomed->reqcap = NULL;
 	ast_stream_topology_free(doomed->reqtopology);
 	doomed->reqtopology = NULL;
+	ao2_cleanup(doomed->owner_old_topology);
+	ao2_cleanup(doomed->chan_old_topology);
 }
 
 struct ast_unreal_pvt *ast_unreal_alloc(size_t size, ao2_destructor_fn destructor, struct ast_format_cap *cap)

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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Id103cc5c9295295d8e68d5628e76220f8f17e9fb
Gerrit-Change-Number: 15362
Gerrit-PatchSet: 4
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210216/ff4e0ac3/attachment-0001.html>