[asterisk-dev] [Code Review] better SDP parsing algorithm for 1.4

[Matthew Nicholson]

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


I think we are almost ready to commit this.  I tested this using sipp and reproduced the problem with stock 1.4 chan_sip and then applied the latest patch which fixed the issue.  I also spent some time reviewing the code, my comments are listed below.  Once this is committed to 1.4 it needs to be merged to 1.6 and trunk.  Let me know if you are willing to assist in this effort.


branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2734>

    Coding guidelines, put a space after 'if'.



branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2731>

    Parens are not necessary here. hp = &audiohp.hp; should do what you want.  This also applies to a few other instances in the code.



branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2733>

    Coding guidlines, put a space after while: while () { ...



branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2735>

    Coding guidelines.



branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2736>

    Coding guidelines.



branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2728>

    This done twice.  Once here and once below.



branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/385/#comment2729>

    Here is the second instance. It looks like the original code does that too. I don't fully understand why.


I noticed that the vsendonly value is never applied to an RTP stream.  Is this intentional?

- Matthew


On 2009-10-15 10:27:41, frawd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/385/
> -----------------------------------------------------------
> 
> (Updated 2009-10-15 10:27:41)
> 
> 
> Review request for Asterisk Developers, Olle E Johansson and Matthew Nicholson.
> 
> 
> Summary
> -------
> 
> This patch cleans up asterisk's SDP parsing algorithm, resolving a few bugs including #14994. It does the parsing line by line, making a distinction between session-level and media-specific parameters. It also optimizes the parsing adding functions for audio/video/image specific scanning.
> I added debug information for a better understanding on how the parsing is actually done (shows each SDP line parsed with OK or UNSUPPORTED).
> 
> If ported to 1.6 (sorry but I have no idea how the SIP code changed between 1.4 and 1.6 so I don't know how to do it), it will allow to easily add SDP functionality.
> 
> 
> This addresses bug 0014994.
>     https://issues.asterisk.org/view.php?id=0014994
> 
> 
> Diffs
> -----
> 
>   branches/1.4/channels/chan_sip.c 224144 
> 
> Diff: https://reviewboard.asterisk.org/r/385/diff
> 
> 
> Testing
> -------
> 
> Production tested for multiple audio and video devices in a version of the patch for 1.4.26.2 (see bug #14994).
> 
> Not tested with T.38, but it should work well.
> 
> 
> Thanks,
> 
> frawd
> 
>