[asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.

[Matt Jordan]

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



/team/group/media_formats-reviewed-trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/3703/#comment22772>

    I think we're going to have a ref leak on format_parsed here. Since ast_format_parse_sdp_fmtp will also return a new format with the reference count increased to 1, the ao2_replace will actually bump this one further than we want.
    
    If I'm counting this correctly:
    
    11305: if ((format = ast_rtp_codecs_get_payload_format(newaudiortp, codec))) { /* +1 to format */
    11309: format_parsed = ast_format_parse_sdp_fmtp(format, fmtp_string); /* +1 to format_parsed */
    11311: ast_rtp_codecs_payload_replace_format(newaudiortp, codec, format_parsed); /* Store format_parsed on codecs, +1 to format_parsed */
    11312: ao2_replace(format, format_parsed); /* -1 to format (matches original ref); +1 to format_parsed; format is now format_parsed */
    11346: ao2_ref(format, -1); /* -1 to format_parsed */
    
    Which leaves format_parsed (now format) with a ref count of 1, when it should be 0 (or neutral) after this routine.
    
    I think the ao2_replace of format with format_parsed should actually be:
    ao2_ref(format, -1);
    format = format_parsed
    
    Which would allow the latter ao2_ref on 11346 to dispose of the reference returned by ast_format_parse_sdp_fmtp
    



/team/group/media_formats-reviewed-trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/3703/#comment22773>

    Same potential leak here


- Matt Jordan


On July 3, 2014, 7:15 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3703/
> -----------------------------------------------------------
> 
> (Updated July 3, 2014, 7:15 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23957
>     https://issues.asterisk.org/jira/browse/ASTERISK-23957
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change does a few things:
> 
> 1. Fixes an issue where direct format interfaces were treated as AO2 objects when they were not.
> 2. Added an ast_format_clone API call which clones and deep copies a format, returning one which can be safely modified.
> 3. Changed the format interface API so anything which manipulates the format returns a new format.
> 4. Added get/set functions for format attribute data.
> 5. Added an API call to replace the format in an RTP engine codecs structure.
> 6. Updated the res_format_attr_* modules to work with the new media formats work.
> 7. Added support for loading an interface module after a format has been created.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/res/res_pjsip_sdp_rtp.c 417804 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c 417804 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_opus.c 417804 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_h264.c 417804 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_h263.c 417804 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c 417804 
>   /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417804 
>   /team/group/media_formats-reviewed-trunk/main/format.c 417804 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h 417804 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417804 
>   /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417804 
> 
> Diff: https://reviewboard.asterisk.org/r/3703/diff/
> 
> 
> Testing
> -------
> 
> Placed calls in and out, confirmed that the attribute stuff doesn't crash things and that received SDP is parsed and interpreted. What doesn't currently work is passing this information through so outgoing calls have the correct attributes.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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