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

[Corey Farrell]

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


I still need review the res_format_attr modules again, hopefully I'll understand them next time.


/team/group/media_formats-reviewed-trunk/include/asterisk/format.h
<https://reviewboard.asterisk.org/r/3703/#comment22704>

    The returned format is a new ao2 object. It must be released using ao2_cleanup.
    
    Since 'format' is the parameter, the comment could be misinterpreted to say that the input parameter is bumped.  Also since this procedure can return NULL I think we should not suggest ao2_ref for release.



/team/group/media_formats-reviewed-trunk/main/format.c
<https://reviewboard.asterisk.org/r/3703/#comment22705>

    Do we need to check for !cloned->interface->format_clone?  Maybe that should happen in __ast_format_interface_register?



/team/group/media_formats-reviewed-trunk/main/format.c
<https://reviewboard.asterisk.org/r/3703/#comment22706>

    I don't understand the reason for this clone.  We've failed to set the requested attribute, I would think we should return NULL.  If we want to return non-NULL why not just return ao2_bump(format)?  If we can't set attributes, the original immutable format should be all we need.
    
    This finding also applies to ast_format_sdp_parse.


- Corey Farrell


On July 2, 2014, 12:23 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3703/
> -----------------------------------------------------------
> 
> (Updated July 2, 2014, 12:23 p.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 417762 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c 417762 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_opus.c 417762 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_h264.c 417762 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_h263.c 417762 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c 417762 
>   /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417762 
>   /team/group/media_formats-reviewed-trunk/main/format.c 417762 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h 417762 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417762 
>   /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417762 
> 
> 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/20140702/7741122d/attachment.html>