[asterisk-dev] [Code Review] 3671: media_formats: res_rtp_asterisk and other fixes

[Matt Jordan]

> On June 25, 2014, 6:45 a.m., Joshua Colp wrote:
> > /team/group/media_formats-reviewed-trunk/main/format.c, lines 200-202
> > <https://reviewboard.asterisk.org/r/3671/diff/1/?file=60620#file60620line200>
> >
> >     Just curious - does this happen?
> 
> Corey Farrell wrote:
>     This could happen if someone configures a blank format_cap (disallow=all) - the first format would be NULL (ast_best_codec).
>     
>     Maybe an assert should go here until we get a better handle on things?  If NULL's to this procedure are possible we probably want to check this second (if both are NULL then they are equal).
> 
> Joshua Colp wrote:
>     My question becomes: Should this accept NULLs or should the caller see that something they expected to be non-NULL is NULL and thus should print out an error/warning/something.

The reason why it is useful to accept NULLs here is explicitly because of res_rtp_asterisk and its silliness. res_rtp_asterisk has to deal both with 'rtp-isms' - things like Cisco specific DTMF - as well Asterisk formats. It stores both of these in the same static array. As such, there are times when it has a format pointer that can be NULL (because the 'format' is Cisco DTMF or something equally silly) but it is running through checks to see if the format is T.140, or G722, or other such things. Having this function gracefully handle NULLs keeps some code in res_rtp_asterisk slightly more sane.

Since this is a heavily used function, it's probably good for it to be as 'safe' as possible. The NULL check here is unlikely to be a performance bottleneck.


- Matt


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


On June 25, 2014, 7:14 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3671/
> -----------------------------------------------------------
> 
> (Updated June 25, 2014, 7:14 a.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Matt Jordan.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This update gives media_formats the ability to receive a call using chan_sip.  Possibly other channel drivers might work, I haven't tried them.
> 
> * ast_format_cap_is_compatible_format needs to be checked against AST_FORMAT_CMP_NOT_EQUAL, not zero/non-zero.  All calls to ast_format_cap_is_compatible_format were fixed.
> * res_rtp_asterisk was updated by Matt Jordan, along with related changes to codec.c, codec.h, format.c, format.c and codec_builtin.c.
> * Switch ast_format_copy from function to macro to ao2_bump.  This allows REF_DEBUG to give better results.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/res/res_speech.c 417190 
>   /team/group/media_formats-reviewed-trunk/res/res_rtp_asterisk.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/translate.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/frame.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/format.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/codec_builtin.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/codec.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/channel.c 417190 
>   /team/group/media_formats-reviewed-trunk/main/bridge.c 417190 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417190 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/codec.h 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_unistim.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_skinny.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_pjsip.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_oss.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_nbs.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_motif.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_mgcp.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_jingle.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_h323.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_gtalk.c 417190 
>   /team/group/media_formats-reviewed-trunk/channels/chan_alsa.c 417190 
>   /team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c 417190 
>   /team/group/media_formats-reviewed-trunk/addons/chan_mobile.c 417190 
> 
> Diff: https://reviewboard.asterisk.org/r/3671/diff/
> 
> 
> Testing
> -------
> 
> Called from Asterisk 11 to a test server with this code, I was able to hear the 'invalid' message, everything seemed during the call.  I received TONS of ao2 frack's when stopping Asterisk.  The sip.conf peer on both Asterisk servers was setup for disallow=all / allow=ulaw.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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