[asterisk-dev] [Code Review] 3275: res_rtp_asterisk: Fix the one way audio problems when resuming held calls with ICE

[Matt Jordan]

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



/branches/11/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/3275/#comment20634>

    For the sake of readability, line up the comments using spaces



/branches/11/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/3275/#comment20633>

    Although this was already named local_candidates, in order to keep the naming consistent with the other containers, you may want to consider renaming this to 'ice_local_candidates' as well.
    
    No worries if you don't want to however.



/branches/11/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/3275/#comment20635>

    This function duplicates a lot of code in ast_rtp_new.
    
    I'd create a new function for the shared code of actually making the ICE session called 'ice_create'. That should actually return a -1/0 for failure/success, as pj_ice_sess_create and technically fail.
    
    If the ICE session creation fails, we should not set ice_started.



/branches/11/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/3275/#comment20637>

    Once you've refactored this code into ice_create, assign rtp->ice_port on success.


- Matt Jordan


On Feb. 27, 2014, 3:53 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3275/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2014, 3:53 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Kevin Harwell, and Matt Jordan.
> 
> 
> Bugs: ASTERISK-22911
>     https://issues.asterisk.org/jira/browse/ASTERISK-22911
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch provides a fix for the hold problem by doing the following:
> 
> Once an ICE session is marked as started, we start adding any new remote candidates into a separate list until we get another attempt to start the ICE session.
> Once a call to start the ice session is made, instead of immediately quitting if the session is already started, we check for a difference in the two candidates lists.  If the lists are identical, we wipe out the new list and keep the old one and just quit then going on with the current ICE session. If the lists are changed, we toss the old list and adopt the new one and restart the ICE session.
> 
> 
> Diffs
> -----
> 
>   /branches/11/res/res_rtp_asterisk.c 409132 
> 
> Diff: https://reviewboard.asterisk.org/r/3275/diff/
> 
> 
> Testing
> -------
> 
> SIPML client to Asterisk to Desk Phone
> SIPML calls desk phone
> audio test, got two way audio
> SIPML holds call
> SIPML resumes call
> audio test, got two way audio (previously this would cause one way audio from the SIPML client to the desk phone)
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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