[Home]

Summary:ASTERISK-08891: [patch] native bridging support for chan_skinny
Reporter:dea (dea)Labels:
Date Opened:2007-02-26 21:07:25.000-0600Date Closed:2007-09-14 14:30:29
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_skinny-native-bridge-v3.txt
( 1) chan_skinny-native-bridge-v4.txt
( 2) chan_skinny-native-bridge-v5.txt
( 3) skinny-native-bridge.txt
( 4) skinny-native-bridge-trunk.txt
( 5) skinny-native-bridge-trunk-v2.txt
Description:Possibly a bit rough, but so far it passes tests for
core and native bridging.  P2P appears to need some
work, but I thought I might have enough to solicit
feedback.

****** ADDITIONAL INFORMATION ******

This is against SVN branches-1.4.
It should applied to trunk.  
Comments:By: Serge Vecher (serge-v) 2007-02-27 10:16:31.000-0600

Good going, Dan ;)
By: dea (dea) 2007-06-01 20:22:46

OK, here's a trunk version.  It has an issue that P2P is always triggered,
but I am running basically the same code against a tweaked 1.4 and it
works.

Extra eyes would be appreciated.  This feature is not the default
behaviour for calls.  The phone/global settings need to set NAT to
no and canreinvite to yes for the feature to be enabled.
By: pj (pj) 2007-06-02 03:05:56

can't test reinvite right now, because I'm behind NAT, so rtp stream must always go through asterisk, so only note: this patch is applied successfully to trunk only some warnings are generated during compile...

  [CC] chan_skinny.c -> chan_skinny.o
chan_skinny.c: In function ?skinny_get_rtp_peer?:
chan_skinny.c:1955: warning: too few arguments for format
chan_skinny.c: In function ?skinny_set_rtp_peer?:
chan_skinny.c:2006: warning: ?us.sin_port? is used uninitialized in this function
  [LD] chan_skinny.o -> chan_skinny.so
By: dea (dea) 2007-06-02 10:54:30

I'm a bonehead.  The error is in a log message that did not need a
parameter.  Just delete the = %s on that line and I'll get another
patch posted.


By: dea (dea) 2007-08-02 19:25:01

Small cleanup effort to keep in step with Trunk.  Tested to work with
7920/7940/CIPC phones to SIP, OH323 and other skinny channels.  

V3 had an issue with some phones.  V4 passes two way audio in
Native/P2P and core bridging tests.


By: Jason Parker (jparker) 2007-08-03 16:44:05

You had said that there was an issue in that it always triggered P2P mode.  Has this been fixed?
By: dea (dea) 2007-08-03 16:53:30

I think it was an issue with me failing to reset one of my SIP peers to
allow re-invite during testing.  I wrote the code against 1.4 and ported
it to Trunk, and lost track of what I had and had not tested.

I re-tested the Trunk patch for the re-invite/no re-invite cases and both
passed.  Another issue was at the time of the first patch rtp.c would P2P
channels with different packetization (which I use in my test environment).
That has since been resolved in Trunk, so I think I have all of the bases
covered.


By: Jason Parker (jparker) 2007-08-03 17:32:35

It appears that sub->canreinvite is unused, and can be removed.

Also, us is being used uninitialized:
req->data.startmedia.remotePort = htolel(ntohs(us.sin_port));
By: dea (dea) 2007-08-03 17:56:53

Correct on sub->canreinvite, that is a hold over from my first pass before
I decided that decision should be based on the line.

It looks like I can add:
ast_rtp_get_us(rtp, &us);

Below the test:
if (!(l->canreinvite) || (l->nat)){

In skinny_set_rtp_peer.  At that point in the code we know we have
an rtp structure, and there is no point setting it fi we are not going to
use it.

Oh, and the additional information is now wrong.  The target is trunk and
not Branches/1.4

** Edit  **
Upload v5 with the sub->canreinvite removed, added the call to ast_rtp_get_us
and added a return value in case of memory allocation failure in
skinny_set_rtp_peer

** Edit 2  **
I will be on vacation next week, so I will not be able to hack on this.
I think it is ready for testing, and will tackle any issues found when I
get back.


By: Andreas Anderson (aanderson) 2007-08-04 06:05:01

Any chances that this will support the native "Wideband 256k" mode?
By: dea (dea) 2007-08-04 11:19:16

This patch does not alter the channels list of supported codecs, or
change the codec negotiation process.  It simply adds the infrastructure
that allows Asterisk to communicate to the skinny phone the IP:port
details of the far-ends media streams.
By: dea (dea) 2007-09-07 19:38:31

Any interest in this, or were people just waiting for me to get
back from vacation?
By: Jason Parker (jparker) 2007-09-14 13:19:40

yeah, I'd still like to commit this if you're interested in finishing/re-reviewing it.
By: dea (dea) 2007-09-14 13:50:24

I'll grab a fresh copy of trunk and see how it fits.  The functionality
passed testing in my environment.  I should have an update shortly.
By: dea (dea) 2007-09-14 14:16:38

Patch V5 applies to SVN Trunk 82400 with fuzz and no rejects.
The code comiples without error or warning and passes the
following tests:

1.  Skinny phone allowed to re-invite (Native)
2.  Skinny phone not allowed to re-invite (P2P)
3.  Skinnt phone allowed, but with non-standard packetization (Core)

Tested against SIP<>Skinny, Skinny<>Skinny


By: Digium Subversion (svnbot) 2007-09-14 14:30:29

Repository: asterisk
Revision: 82401

------------------------------------------------------------------------
r82401 | qwell | 2007-09-14 14:30:25 -0500 (Fri, 14 Sep 2007) | 4 lines

Add support in chan_skinny for sending RTP directly to the endpoints.

Closes issue ASTERISK-8891, patch by DEA

------------------------------------------------------------------------