Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/ --- (Updated July 9, 2014, 5:51 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 418248 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
---
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
--
_
-- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
On July 7, 2014, 10:50 a.m., Matt Jordan wrote: The findings here were resolved in https://reviewboard.asterisk.org/r/3722 - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/#review12506 --- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
On July 2, 2014, 3:25 p.m., Corey Farrell wrote: /team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c, lines 48-53 https://reviewboard.asterisk.org/r/3703/diff/2/?file=62050#file62050line48 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? I'm not sure I agree with this finding. Right now, the format attribute modules are completely responsible for managing attributes. That has the benefit of putting all the logic in a single module. Granted, right now, attributes are relatively simple structures, but there's no guarantee that will always be the case - and this design is more flexible for the future. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/#review12443 --- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/ --- (Updated July 3, 2014, 12:15 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 (updated) - /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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
On July 2, 2014, 8:25 p.m., Corey Farrell wrote: /team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c, lines 269-281 https://reviewboard.asterisk.org/r/3703/diff/2/?file=62054#file62054line269 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. These modules haven't changed since they were implemented really, I think going forward any future modules should evaluate whether that is a good idea or not. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/#review12443 --- On July 3, 2014, 12:15 p.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/ --- (Updated July 3, 2014, 12:15 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 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 -- _ -- 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
[asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/ --- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
--- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/ --- (Updated July 2, 2014, 7: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 (updated) - /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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
--- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
On July 2, 2014, 8:25 p.m., Corey Farrell wrote: /team/group/media_formats-reviewed-trunk/include/asterisk/format.h, lines 51-60 https://reviewboard.asterisk.org/r/3703/diff/2/?file=62046#file62046line51 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. That makes no attribute_data no longer opaque, you assume it is something that can just be copied like that. I'm not sure I'm okay with that. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/#review12443 --- On July 2, 2014, 7: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, 7: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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3703: media_formats: Move format attribute modules over, tweak API, and fix some bugs.
On July 2, 2014, 4:25 p.m., Corey Farrell wrote: /team/group/media_formats-reviewed-trunk/include/asterisk/format.h, lines 51-60 https://reviewboard.asterisk.org/r/3703/diff/2/?file=62046#file62046line51 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. Joshua Colp wrote: That makes no attribute_data no longer opaque, you assume it is something that can just be copied like that. I'm not sure I'm okay with that. I was looking at attribute_data as an opaque structure of a fixed size per ast_format_interface. Unless you feel that we need to support variable sized attribute_data then I don't see this is an issue. The optional callback would allow a format_attr module to reallocate pointers or bump ao2's in attribute_data fields. - Corey --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3703/#review12443 --- 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 -- _ -- 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
