[asterisk-dev] [Code Review] 3763: media formats: Fix appending of compatible formats

[Matt Jordan]

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



team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/3763/#comment22902>

    Beyond my other finding, I'm not sure why this change is necessary.
    
    When rtp_engine calls get_codecs, it is passing in an empty capabilities structure. There is nothing in the capabilities structure for ast_channel_nativeformats to be compatible *with*, hence, merely appending them over should be sufficient.



team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/3763/#comment22901>

    I don't think this pattern is safe. You're treating a format capability structure as both const and non-const.
    
    When ast_format_cap_get_compatible does its work, it will iterate over ast_channel_nativeformats(chan), looking up each format in result. When it finds one, it will place that into... result.
    
    Modifying something in place like this feels prone to odd behaviour. I'd much rather see us pass in a new format capability structure that is known to be empty, and set result to that.


- Matt Jordan


On July 14, 2014, 9:42 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3763/
> -----------------------------------------------------------
> 
> (Updated July 14, 2014, 9:42 a.m.)
> 
> 
> Review request for Asterisk Developers, Corey Farrell and Matt Jordan.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When ast_format_cap_append was replaced with ast_format_cap_append_from_cap, the behavior of the original function was not preserved. This change introduces ast_format_cap_append_compatible_from_cap which reproduces the original behavior of ast_format_cap_append and alters the original users of ast_format_cap_append to use ast_format_cap_append_compatible_from_cap.
> 
> The primary difference in behavior is that ast_format_cap_append_from_cap appends unconditionally from the source format capability while ast_format_cap_append checks for compatibility with ast_format_cap_iscompatible.
> 
> 
> Diffs
> -----
> 
>   team/group/media_formats-reviewed-trunk/res/res_pjsip_session.c 418507 
>   team/group/media_formats-reviewed-trunk/res/res_pjsip_sdp_rtp.c 418507 
>   team/group/media_formats-reviewed-trunk/channels/chan_sip.c 418507 
>   team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c 418507 
> 
> Diff: https://reviewboard.asterisk.org/r/3763/diff/
> 
> 
> Testing
> -------
> 
> Ensured that calling a channel via Dial() produced the same behavior as trunk.
> 
> 
> Thanks,
> 
> opticron
> 
>

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