-----------------------------------------------------------
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
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
