[asterisk-dev] [Code Review] 3256: res_rtp_asterisk: Undo regression from ASTERISK-23213 while working around PJNATH assertion abort from ASTERISK-22911

Thu Feb 27 10:38:20 CST 2014

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

(Updated Feb. 27, 2014, 10:38 a.m.)

Review request for Asterisk Developers, Joshua Colp, Kevin Harwell, and Matt Jordan.

Changes
-------

Attached is jcolp's patch for fixing the SRFLX address local candidate base addresses. This should fix the problems that were occurring while pruning the checklist from PJNATH's create check list function and since that was causing the creation of the checklists to fail, this should ultimately fix the regression that was occurring in ASTERISK-23213 in a much more elegant way than the patch I wrote did.

However, there are currently still problems with holds from the ICE clients with this patch that leave us without audio. In order to fix this, we need to toss the old ICE session and start a new one when an ICE session sends new remote candidates. I have a separate effort going on this since it seems like a separate issue, but I'm running into trouble with completing the start of the new ICE session when resuming the call and this is leaving me with one way audio again.

Possibly worth of note:
With just this patch, my one way audio ends up being from my SIPML client using ICE into Asterisk.
With the patch that creates a new ICE session, my one way audio flows in the opposite direction. Asterisk sends audio and the SIPML5 client receives it, but the SIPML5 client sends no audio back in return.

In any event, this patch should be sufficient for fixing ASTERISK-23213 and I'll be requesting that the participants on that issue test it as well.

Bugs: ASTERISK-22911 and ASTERISK-23213
    https://issues.asterisk.org/jira/browse/ASTERISK-22911
    https://issues.asterisk.org/jira/browse/ASTERISK-23213

Repository: Asterisk

Description
-------

Let me start by saying this is almost certainly not the complete answer to solving these problems. This patch is simply an alternative to backing out the patch from r405234 and leaving the existing aborts in place. I've written a test to make sure the new patch (and likely a later patch which can resolve these problems with ICE more comprehensively) does not crash Asterisk and that can be view here: https://reviewboard.asterisk.org/r/3255/

In my reproduction of this regression, I noticed a few things. The first was that when starting a call with ICE from SIPML5 that Asterisk would not be able to full initialize the ICE session. Instead when creating the candidate checklist via pj_ice_sess_create_check_list, PJNATH would be unable to associate the srflx candidates with any host pairs when pruning the checklist. The SRFLX candidates would have the addresses used internally on my LAN while the addresses it would be matched up against would mirror those of my external IP (in other words, they just didn't match by address). The overall return from pj_ice_sess_create_check_list would be PJNATH_EICENOHOSTCAND and while the ICE session wouldn't get flagged as started, I still continued to receive two way audio on both ends in Asterisk 11.7

Asterisk 11.8 however would clear the candidate lists at this point and I would get one way audio instead.

The crash in 11.7 from holding was caused because upon holding the call, the SDP would contain the same list of ICE candidates, so Asterisk would attempt to start the ICE session again. This time when it entered the create check list function, the maximum ICE candidates in the session would be exhausted causing PJNATH to assert and abort (which visibly appears more or less the same as a crash). This work around patch checks to see if a create checklist call will cause that value to expand above the maximum and if it would, we abandon starting ICE back up and we clear the current candidate list on the ICE session. In my own tests this seemed to work quite well (I had two way audio, Asterisk didn't terminate suddenly, and I was able to hold and resume the call while still maintaining two way audio and music on hold for all expected ends). According to Vytis though, neither this patch nor the original patch to resolve ASTERISK-22911 by kharwell actually fixed his audio problems anyway. Kharwell's patch fixed the assertions that were occurring because the candidate list would be cleared on any error including the PJNATH_EICENOHOSTCAND which most of the people in ASTERISK-23213 were most likely experiencing as well. I draw that conclusion because when they used this patch they reported that it fixed their issues without introducing any new crashes.

At this point, I'm not entirely sure that audio actually should be allowed to work when building the ICE session check list fails like it has been in this scenario. But we need to finalize a new Asterisk 11 release soon and we can't have this apparent regression in when we do so. In the future we need to find out more comprehensively why pj_ice_sess_create_check_list is failing and how to fix that.

Props to Lott Caskey for providing some improvements to the patch that I wrote.

Diffs (updated)
-----

  /branches/11/res/res_rtp_asterisk.c 408284 

Diff: https://reviewboard.asterisk.org/r/3256/diff/

Testing
-------

Tested calls and holds with a SIPML5 call to a desk phone. Confirmed two way audio and music on hold.
Requested that the reporter and participants of ASTERISK-23213 test the patch. Everyone who tested it confirmed that it fixed their problems.
Ran https://reviewboard.asterisk.org/r/3255/ and checked the log files to see what was happening

Thanks,

Jonathan Rose

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140227/b16a72fb/attachment-0001.html>