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

[Jonathan Rose]

> On July 11, 2014, 4:18 p.m., opticron wrote:
> > /team/group/media_formats-reviewed-trunk/channels/chan_iax2.c, line 6023
> > <https://reviewboard.asterisk.org/r/3747/diff/2/?file=62811#file62811line6023>
> >
> >     I had a similar situation in the RTP engine and the solution was to figure out why the format is NULL. Should the format ever reasonably be NULL here?
> 
> Jonathan Rose wrote:
>     I'm inclined to say this is reasonable because when the format was NULL when entering this function, it was usually an AST_FRAME_IAX type frame. To be blunt though, I'm not sure if that really means it shouldn't have a format.
> 
> Jonathan Rose wrote:
>     dug around a little.  Format is only set for voice frames, so it's really not necessary to set it at this point. I have to set it to something just because of the way this function is set up, but for now I'm initializing it to 0 and then setting it to ast_format_get_sample_rate(f->subclass.format) on the first check to see if it's a voice frame. That appears still fixes the crash that was occurring earlier with this function.

Clarification:
Well, format may be set for video frames as well.  In this particular function it is only being *used* for voice frames.


- Jonathan


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


On July 11, 2014, 3:59 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3747/
> -----------------------------------------------------------
> 
> (Updated July 11, 2014, 3:59 p.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/20140711/509cdb51/attachment-0001.html>