[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/#review12443
-----------------------------------------------------------



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

    Every format_clone function is identical, except for the obvious adjustment for sizeof(*attr).  I think it would be better for ast_format_interface to store sizeof(*attr).  Then make ast_format_clone just handle copying the attribute data.  We could keep format_clone as an optional callback for situations where a deep copy is required - like if attribute has a pointer that must be deep copied.



/team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c
<https://reviewboard.asterisk.org/r/3703/#comment22711>

    All format_destroy callbacks are identical.  I think it would be better to make format.c responsible for freeing attribute data after calling the (optional) format_destroy callback.  This callback would exist in-case the attributes contain a pointer that must be free'd or reference released.  Maybe also rename the callback to format_cleanup?



/team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c
<https://reviewboard.asterisk.org/r/3703/#comment22710>

    format_attribute_set in opus / silk have sscanf once in the top of the procedure.  Unless you have a reason that can't be done here it would be better (cleaner).



/team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c
<https://reviewboard.asterisk.org/r/3703/#comment22708>

    Just a thought, what if this used the ACO model of creating an ao2_container *attr_offset to lookup the field offset's?  Not sure if that would be more or less efficient, but would look cleaner / easier to maintain.
    
    This idea applies to all format_attribute_set callbacks.


- Corey Farrell


On July 2, 2014, 3:24 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, 3:24 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 417769 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c 417769 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_opus.c 417769 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_h264.c 417769 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_h263.c 417769 
>   /team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c 417769 
>   /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417769 
>   /team/group/media_formats-reviewed-trunk/main/format.c 417769 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h 417769 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417769 
>   /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417769 
> 
> 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/1f1c79ed/attachment-0001.html>