----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
