[asterisk-dev] [Code Review] 3747: media formats: Get chan_iax2 runnable and usable in the media formats branch

[Matt Jordan]

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



/team/group/media_formats-reviewed-trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/3747/#comment22905>

    This should probably check for the failure of this function. If it fails, you probably need to bail out of this function and return 1.



/team/group/media_formats-reviewed-trunk/main/format_compatibility.c
<https://reviewboard.asterisk.org/r/3747/#comment22906>

    You could write this instead as:
    
    for (x = 0; x < AST_CODEC_PREF_SIZE && x < size; x++) {
    ...
    }



/team/group/media_formats-reviewed-trunk/main/format_compatibility.c
<https://reviewboard.asterisk.org/r/3747/#comment22907>

    Same here:
    
    for (x = 0; x < size && x < AST_CODEC_PREF_SIZE; x++)


- Matt Jordan


On July 14, 2014, 11:42 a.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3747/
> -----------------------------------------------------------
> 
> (Updated July 14, 2014, 11:42 a.m.)
> 
> 
> Review request for Asterisk Developers, Corey Farrell, Joshua Colp, Matt Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-23958
>     https://issues.asterisk.org/jira/browse/ASTERISK-23958
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Hmmm, where to begin...
> 
> The media formats branch currently has a lot of channels in a less than usable state.  This patch brings chan_iax2 back to being usable, but there are still problems remaining (right now I'm working on codec negotiation).
> 
> First, chan_iax2 wouldn't build because ast_codec_pref_convert wasn't defined. After fixing that function to get it to work again, I realized that codecs weren't being set for IAX peers/users because the iax2_parse_allow_disallow function wasn't actually saving the formats between passes, just the format preferences. Also the preferences would get out of order because every time it entered a new allow/disallow line, the preferences would be converted into an ast_format_cap without order information preserved and then re-added. The ast_codec_pref_append function wasn't working how I expected it to and would sometimes double up the same pref or delete in a semi-random way. Looking into that the ast_codec_remove function wasn't deleting existing codecs in a way that could be considered proper. That function got mostly rewritten and I think it's a good bit simpler now.
> 
> Some functions were abusing formats pointers declared on the stack and treating them as if they were pointing at alloc'd format structs. I ended up rewriting the ast_codec_pref_string function as well since it was doing some of that format abuse.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/main/format_compatibility.c 418208 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_compatibility.h 418208 
>   /team/group/media_formats-reviewed-trunk/channels/iax2/parser.c 418208 
>   /team/group/media_formats-reviewed-trunk/channels/chan_iax2.c 418208 
> 
> Diff: https://reviewboard.asterisk.org/r/3747/diff/
> 
> 
> Testing
> -------
> 
> Configured two IAX peers
> 
> [lappy]
> type=friend
> requirecalltoken = no
> username = lappy
> secret = secret
> host = dynamic
> dtmfmode = auto
> transfer = no
> encryption = no
> qualify = 300
> disallow = all
> allow=ulaw
> allow=gsm
> allow=alaw
> context = default
> 
> [deskbox]
> type=friend
> requirecalltoken = no
> username = deskbox
> secret = secret
> host = dynamic
> transfer = no
> dtmfmode = auto
> encryption = no
> qualify = 300
> disallow = all
> allow=ulaw
> allow=alaw
> allow=gsm
> context = default
> 
> Made sure they were configured in a way that the prefs order values were the same as in current builds of Asterisk 12. Speaking of which, I'm not sure why the arrays were changed from integers to uint64_t... that indicates to me that the idea might have been to store the bitflag representations of the formats in these structs, but little was changed to account for that difference, so I'm not really sure.  As a result I had to make a few functions for going back and forth between the format indices that chan_iax2 was using in the order values and the format bitfield representations that can be used to add to ast_format_cap structs.
> 
> Made sure the output for ast_codec_pref_string would have the same order as allow/disallow options specified. Made sure that if the buffer for ast_codec_pref_string was less than what was needed to write the full string that it would be terminated early in a predictable way.
> 
> Made sure the ast_codec_pref_convert function wrote the same strings to buf when right=1 as they did in Asterisk 12.  Still haven't verified how it works in right=0 mode.
> 
> Made sure that I could make a call from one IAX softphone on my laptop to an IAX softphone on my desktop.  Received audio going both ways.  Codec preferences didn't have an effect unfortunately while they did in twelve. Instead, all audio chosen was GSM, presumably because it has the lowest format value. I'm still looking into that.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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