[asterisk-dev] [Code Review]: chan_jingle2: New Jingle + Google Talk channel driver

[Joshua Colp]

> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 12
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line12>
> >
> >     I think you need to s/gmail/Gmail/

Changed.


> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 14
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line14>
> >
> >     s/used/uses/
> >     And I'd drop the word "very"

Changed.


> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 35
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line35>
> >
> >     s/searched/searched for/

Changed.


> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 40
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line40>
> >
> >     s/transpor/transport/

Fixed.


> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 58
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line58>
> >
> >     I don't like "settable" so I'd say:
> >     
> >     Outgoing caller id can *not* be set.

Changed.


> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 62
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line62>
> >
> >     Looks like you perhaps meant to add a comment after disallow=all? Or maybe just a spare semi-colon.

Removed.


> On May 14, 2012, 8:13 a.m., Leif Madsen wrote:
> > /trunk/configs/jingle2.conf.sample, line 68
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27834#file27834line68>
> >
> >     I don't think I like that you're using the 'default' context for your end points. You're kind of directing people to allow outgoing calls in the 'default' context, which is not good. Should probably at least use something that isn't defined generally for unauthenticated calls.

I've changed this to an incoming-jingle2 context. These are unauthenticated calls until you decide they are "authenticated" based on the values in CallerID.


- Joshua


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1917/#review6209
-----------------------------------------------------------


On May 13, 2012, 12:15 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1917/
> -----------------------------------------------------------
> 
> (Updated May 13, 2012, 12:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a new channel driver written from scratch for the Jingle, Google Jingle, and Google Talk protocols. It has been written to the specs available and tested extensively.
> 
> ICE and STUN support for Jingle uses the new ICE/STUN/TURN support which is present in another review. (Please do not review any of that code in this review)
> STUN support for Google uses the existing STUN implementation, as the new support is not compatible with it.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_jingle2.c PRE-CREATION 
>   /trunk/channels/chan_sip.c 365451 
>   /trunk/configs/jingle2.conf.sample PRE-CREATION 
>   /trunk/configs/rtp.conf.sample 365451 
>   /trunk/include/asterisk/jabber.h 365451 
>   /trunk/include/asterisk/jingle.h 365451 
>   /trunk/include/asterisk/rtp_engine.h 365451 
>   /trunk/main/rtp_engine.c 365451 
>   /trunk/res/Makefile 365451 
>   /trunk/res/res_jabber.c 365451 
>   /trunk/res/res_rtp_asterisk.c 365451 
> 
> Diff: https://reviewboard.asterisk.org/r/1917/diff
> 
> 
> Testing
> -------
> 
> Tested audio calls with following:
> 
> GMail Google Talk Plug-in (and video)
> Google Voice
> Jitsi (and video)
> Psi
> OneTeam
> 
> * Included varying codecs (ulaw, speex, g722, etc)
> 
> Tested ringing, hold, and unhold with following:
> 
> Jitsi
> 
> Other clients do not support this.
> 
> 
> Thanks,
> 
> Joshua
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120518/4372c582/attachment.htm>