Review Board 1.7.16


Disallow native RTP bridging when packetization differs between streams

Review Request #2319 - Created Feb. 6, 2013 and submitted

Mark Michelson
/branches/1.8/
ASTERISK-20650
Reviewers
asterisk-dev
Asterisk
In the linked issue, it is brought up that faxing was broken in Asterisk 1.8 due to the fact that Asterisk was not honoring the packetization specified in an SDP. As it turns out, the reason why things went awry was that a local RTP bridge was being used, thus bypassing any smoothers created on the RTP streams.

In order to fix this, I propose this fix. With this, there can be no native RTP bridging (local or remote) if the packetization of the streams is not compatible. If packetization is not the same on both sides of the call, then the bridging must go through the core.

I'm looking for three pieces of feedback here

1) Is this approach correct? Should we prevent native RTP bridging if packetization of the streams differs?
2) Is the code implemented correctly?
3) Should this change be thrown into 1.8 as is? I think this is a bug myself, so I was planning to put this into 1.8. However, I would be interested in knowing if this sort of behavior change could cause unintended consequences.
I tested this by placing a call from SIPp through Asterisk to a Polycom phone on my desk. The SIPp scenario specifies a ptime of 30. The leg between Asterisk and the phone specifies a ptime of 20. Using wireshark, I can see that before this patch is applied, the different packetizations allow for a local RTP bridge to be used. A packet capture shows that audio from Asterisk to SIPp comes in 20 ms segments. With the patch applied, local RTP bridging is prevented. A packet capture shows that audio from Asterisk to SIPp comes in 30 ms segments.

Diff revision 1 (Latest)

  1. /branches/1.8/main/rtp_engine.c: Loading...
/branches/1.8/main/rtp_engine.c
Revision 380922 New Change
[20] 1270 lines
[+20] [+] enum ast_bridge_result ast_rtp_instance_bridge(struct ast_channel *c0, struct ast_channel *c1, int flags, struct ast_frame **fo, struct ast_channel **rc, int timeoutms)
1271
	enum ast_rtp_glue_result audio_glue1_res = AST_RTP_GLUE_RESULT_FORBID, video_glue1_res = AST_RTP_GLUE_RESULT_FORBID;
1271
	enum ast_rtp_glue_result audio_glue1_res = AST_RTP_GLUE_RESULT_FORBID, video_glue1_res = AST_RTP_GLUE_RESULT_FORBID;
1272
	enum ast_bridge_result res = AST_BRIDGE_FAILED;
1272
	enum ast_bridge_result res = AST_BRIDGE_FAILED;
1273
	enum ast_rtp_dtmf_mode dmode;
1273
	enum ast_rtp_dtmf_mode dmode;
1274
	format_t codec0 = 0, codec1 = 0;
1274
	format_t codec0 = 0, codec1 = 0;
1275
	int unlock_chans = 1;
1275
	int unlock_chans = 1;

    
   
1276
	int read_ptime0, read_ptime1, write_ptime0, write_ptime1;
1276

    
   
1277

   
1277
	/* Lock both channels so we can look for the glue that binds them together */
1278
	/* Lock both channels so we can look for the glue that binds them together */
1278
	ast_channel_lock(c0);
1279
	ast_channel_lock(c0);
1279
	while (ast_channel_trylock(c1)) {
1280
	while (ast_channel_trylock(c1)) {
1280
		ast_channel_unlock(c0);
1281
		ast_channel_unlock(c0);
[+20] [20] 69 lines
[+20] enum ast_bridge_result ast_rtp_instance_bridge(struct ast_channel *c0, struct ast_channel *c1, int flags, struct ast_frame **fo, struct ast_channel **rc, int timeoutms)
1350
		ast_debug(1, "Channel codec0 = %s is not codec1 = %s, cannot native bridge in RTP.\n", ast_getformatname(codec0), ast_getformatname(codec1));
1351
		ast_debug(1, "Channel codec0 = %s is not codec1 = %s, cannot native bridge in RTP.\n", ast_getformatname(codec0), ast_getformatname(codec1));
1351
		res = AST_BRIDGE_FAILED_NOWARN;
1352
		res = AST_BRIDGE_FAILED_NOWARN;
1352
		goto done;
1353
		goto done;
1353
	}
1354
	}
1354

    
   
1355

   

    
   
1356
	read_ptime0 = (ast_codec_pref_getsize(&instance0->codecs.pref, c0->rawreadformat)).cur_ms;

    
   
1357
	read_ptime1 = (ast_codec_pref_getsize(&instance1->codecs.pref, c1->rawreadformat)).cur_ms;

    
   
1358
	write_ptime0 = (ast_codec_pref_getsize(&instance0->codecs.pref, c0->rawwriteformat)).cur_ms;

    
   
1359
	write_ptime1 = (ast_codec_pref_getsize(&instance1->codecs.pref, c1->rawwriteformat)).cur_ms;

    
   
1360

   

    
   
1361
	if (read_ptime0 != write_ptime1 || read_ptime1 != write_ptime0) {

    
   
1362
		ast_debug(1, "Packetization differs between RTP streams (%d != %d or %d != %d). Cannot native bridge in RTP\n",

    
   
1363
				read_ptime0, write_ptime1, read_ptime1, write_ptime0);

    
   
1364
		res = AST_BRIDGE_FAILED_NOWARN;

    
   
1365
		goto done;

    
   
1366
	}

    
   
1367

   
1355
	instance0->glue = glue0;
1368
	instance0->glue = glue0;
1356
	instance1->glue = glue1;
1369
	instance1->glue = glue1;
1357
	instance0->chan = c0;
1370
	instance0->chan = c0;
1358
	instance1->chan = c1;
1371
	instance1->chan = c1;
1359

    
   
1372

   
[+20] [20] 474 lines
  1. /branches/1.8/main/rtp_engine.c: Loading...

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.