ARI/PJSIP: Apply requesting channel's format cap to created channels

This patch addresses the following problems:
* ari/resource_channels: In ARI, we currently create a format capability
  structure of SLIN and apply it to the new channel being created. This was
  originally done when the PBX core was used to create the channel, as there
  was a condition where a newly created channel could be created without any
  formats. Unfortunately, now that the Dial API is being used, this has two
  drawbacks:
  (a) SLIN, while it will ensure audio will flows, can cause a lot of
      needless transcodings to occur, particularly when a Local channel is
      created to the dialplan. When no format capabilities are available, the
      Dial API handles this better by handing all audio formats to the requsted
      channels. As such, we defer to that API to provide the format
      capabilities.
  (b) If a channel (requester) is causing this channel to be created, we
      currently don't use its format capabilities as we are passing in our own.
      However, the Dial API will use the requester channel's formats if none
      are passed into it, and the requester channel exists and has format
      capabilities. This is the "best" scenario, as it is the most likely to
      create a media path that minimizes transcoding.
  Fixing this simply entails removing the providing of the format capabilities
  structure to the Dial API.

* chan_pjsip: Rather than blindly picking the first format in the format
  capability structure - which actually *can* be a video or text format - we
  select an audio format, and only pick the first format if that fails. That
  minimizes the weird scenario where we attempt to transcode between video/audio.

* res_pjsip_sdp_rtp: Applied the joint capapbilites to the format structure.
  Since ast_request already limits us down to one format capability once the
  format capabilities are passed along, there's no reason to squelch it here.

* channel: Fixed a comment. The reason we have to minimize our requested
  format capabilities down to a single format is due to Asterisk's inability
  to convey the format to be used back "up" a channel chain. Consider the
  following:

    PJSIP/A => L;1 <=> L;2 => PJSIP/B
    g,u,a     g,u,a    g,u,a      u

  That is, we have PJSIP/A dialing a Local channel, where the Local;2 dials
  PJSIP/B. PJSIP/A has native format capabilities g722,ulaw,alaw; the Local
  channel has inherited those format capabilities down the line; PJSIP/B
  supports only ulaw. According to these format capabilities, ulaw is
  acceptable and should be selected across all the channels, and no
  transcoding should occur. However, there is no way to convey this: when L;2
  and PJSIP/B are put into a bridge, we will select ulaw, but that is not
  conveyed to PJSIP/A and L;1. Thus, we end up with:

    PJSIP/A <=> L;1 <=> L;2 <=> PJSIP/B
      g          g   X   u        u

  Which causes g722 to be written to PJSIP/B.

  Even if we can convey the 'ulaw' choice back up the chain (which through
  some severe hacking in Local channels was accomplished), such that the chain
  looks like:

    PJSIP/A <=> L;1 <=> L;2 <=> PJSIP/B
      u          u       u         u

  We have no way to tell PJSIP/A's *channel driver* to Answer in the SDP back
  with only 'ulaw'. This results in all the channel structures being set up
  correctly, but PJSIP/A *still* sending g722 and causing the chain to fall
  apart.

  There's a lot of difficulty just in setting this up, as there are numerous
  race conditions in the act of bridging, and no clean mechanism to pass the
  selected format backwards down an established channel chain. As such, the
  best that can be done at this point in time is clarifying the comment.

Review: https://reviewboard.asterisk.org/r/4434/

ASTERISK-24812 #close
Reported by: Matt Jordan



git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/13@432195 65c4cc65-6c06-0410-ace0-fbb531ad65f3

4 files changed, 14 insertions, 39 deletions

diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 5476fa66c..18c9ed30c 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -418,12 +418,13 @@ static struct ast_channel *chan_pjsip_new(struct ast_sip_session *session, int s
 	ast_channel_nativeformats_set(chan, caps);
 
 	if (!ast_format_cap_empty(caps)) {
-		/*
-		 * XXX Probably should pick the first audio codec instead
-		 * of simply the first codec.  The first codec may be video.
-		 */
-		struct ast_format *fmt = ast_format_cap_get_format(caps, 0);
+		struct ast_format *fmt;
 
+		fmt = ast_format_cap_get_best_by_type(caps, AST_MEDIA_TYPE_AUDIO);
+		if (!fmt) {
+			/* Since our capabilities aren't empty, this will succeed */
+			fmt = ast_format_cap_get_format(caps, 0);
+		}
 		ast_channel_set_writeformat(chan, fmt);
 		ast_channel_set_rawwriteformat(chan, fmt);
 		ast_channel_set_readformat(chan, fmt);
diff --git a/main/channel.c b/main/channel.c
index da0ca558b..09b8868cf 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -5883,9 +5883,10 @@ struct ast_channel *ast_request(const char *type, struct ast_format_cap *request
 			return NULL;
 
 		/* XXX Only the audio format calculated as being the best for translation
-		 * purposes is used for the request. This needs to be re-evaluated.  It may be
-		 * a better choice to send all the audio formats capable of being translated
-		 * during the request and allow the channel drivers to pick the best one. */
+		 * purposes is used for the request. This is because we don't have the ability
+		 * to signal to the initiator which one of their codecs that was offered is
+		 * the one that was selected, particularly in a chain of Local channels.
+		 */
 		joint_cap = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
 		if (!joint_cap) {
 			return NULL;
diff --git a/res/ari/resource_channels.c b/res/ari/resource_channels.c
index edd319d37..693835129 100644
--- a/res/ari/resource_channels.c
+++ b/res/ari/resource_channels.c
@@ -917,8 +917,6 @@ static void ari_channels_handle_originate_with_id(const char *args_endpoint,
 	char *caller_id = NULL;
 	char *cid_num = NULL;
 	char *cid_name = NULL;
-	RAII_VAR(struct ast_format_cap *, cap,
-		ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);
 	char *stuff;
 	struct ast_channel *other = NULL;
 	struct ast_channel *chan = NULL;
@@ -930,12 +928,6 @@ static void ari_channels_handle_originate_with_id(const char *args_endpoint,
 	struct ari_origination *origination;
 	pthread_t thread;
 
-	if (!cap) {
-		ast_ari_response_alloc_failed(response);
-		return;
-	}
-	ast_format_cap_append(cap, ast_format_slin, 0);
-
 	if ((assignedids.uniqueid && AST_MAX_PUBLIC_UNIQUEID < strlen(assignedids.uniqueid))
 		|| (assignedids.uniqueid2 && AST_MAX_PUBLIC_UNIQUEID < strlen(assignedids.uniqueid2))) {
 		ast_ari_response_error(response, 400, "Bad Request",
@@ -1071,7 +1063,7 @@ static void ari_channels_handle_originate_with_id(const char *args_endpoint,
 		}
 	}
 
-	if (ast_dial_prerun(dial, other, cap)) {
+	if (ast_dial_prerun(dial, other, NULL)) {
 		ast_ari_response_alloc_failed(response);
 		ast_dial_destroy(dial);
 		ast_free(origination);
diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c
index 63a78caa8..dde8f367c 100644
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -240,8 +240,8 @@ static int set_caps(struct ast_sip_session *session, struct ast_sip_session_medi
 	/* get the joint capabilities between peer and endpoint */
 	ast_format_cap_get_compatible(caps, peer, joint);
 	if (!ast_format_cap_count(joint)) {
-		struct ast_str *usbuf = ast_str_alloca(64);
-		struct ast_str *thembuf = ast_str_alloca(64);
+		struct ast_str *usbuf = ast_str_alloca(256);
+		struct ast_str *thembuf = ast_str_alloca(256);
 
 		ast_rtp_codecs_payloads_destroy(&codecs);
 		ast_log(LOG_NOTICE, "No joint capabilities for '%s' media stream between our configuration(%s) and incoming SDP(%s)\n",
@@ -257,36 +257,17 @@ static int set_caps(struct ast_sip_session *session, struct ast_sip_session_medi
 	ast_format_cap_append_from_cap(session->req_caps, joint, AST_MEDIA_TYPE_UNKNOWN);
 
 	if (session->channel) {
-		struct ast_format *fmt;
 
 		ast_channel_lock(session->channel);
-		ast_format_cap_remove_by_type(caps, AST_MEDIA_TYPE_UNKNOWN);
-		ast_format_cap_append_from_cap(caps, ast_channel_nativeformats(session->channel), AST_MEDIA_TYPE_UNKNOWN);
-		ast_format_cap_remove_by_type(caps, media_type);
-
-		/*
-		 * XXX Historically we picked the "best" joint format to use
-		 * and stuck with it.  It would be nice to just append the
-		 * determined joint media capabilities to give translation
-		 * more formats to choose from when necessary.  Unfortunately,
-		 * there are some areas of the system where this doesn't work
-		 * very well. (The softmix bridge in particular is reluctant
-		 * to pick higher fidelity formats and has a problem with
-		 * asymmetric sample rates.)
-		 */
-		fmt = ast_format_cap_get_format(joint, 0);
-		ast_format_cap_append(caps, fmt, 0);
 
 		/*
 		 * Apply the new formats to the channel, potentially changing
 		 * raw read/write formats and translation path while doing so.
 		 */
-		ast_channel_nativeformats_set(session->channel, caps);
+		ast_channel_nativeformats_set(session->channel, joint);
 		ast_set_read_format(session->channel, ast_channel_readformat(session->channel));
 		ast_set_write_format(session->channel, ast_channel_writeformat(session->channel));
 		ast_channel_unlock(session->channel);
-
-		ao2_ref(fmt, -1);
 	}
 
 	ast_rtp_codecs_payloads_destroy(&codecs);