<div dir="ltr">For a few functions you are recommending we create a new function, and keep the old function.  I think we should delete the old function, at most provide a macro so the new function can be called with the old name.  The media formats update breaks ABI, and in many ways API.  Any 3rd party code that is not recompiled after the media_formats merge will not work, and most source will have to be updated to even compile against the new API's.  The lack of compatibility symbols will increase the chances that an incompatible module will not load on and crash asterisk.  I think we should not make any attempt to provide ABI compatibility functions, and should only provide API compatibility functions if not doing so would be a major ordeal.  Changing the parameters of a heavily used function is an example of where I could support a compatibility macro.  For simple renames I say out with the old.<div class="gmail_extra">
<br><br><div class="gmail_quote">On Sat, Jun 21, 2014 at 6:10 PM, Matthew Jordan <span dir="ltr"><<a href="mailto:mjordan@digium.com" target="_blank">mjordan@digium.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hey all -<br>
<br>
I'm working on ASTERISK-23715 [1], which is a standardization/renaming<br>
of public functions/structures affected by the media format work (in<br>
the group/media_formats-reviewed branch). Two functions are called out<br>
on ASTERISK-23715, but I've gone through the other major public header<br>
files and made notes on other functions that I think could either use<br>
renaming, could be removed, or just other general tweaks.<br>
<br>
As an aside, I personally generally prefer the following nomenclature<br>
for naming:<br>
<br>
{namespace}_{object/unit}_{verb}_{target}<br>
<br>
For example, if I wanted a function that retrieved an identifier from<br>
a format, I would expect it to be named:<br>
<br>
ast_format_get_id<br>
<br>
Things get a bit more 'loose' when the item may exist in a particular<br>
header, but may not fit well as the target of the verb. For example,<br>
the format interface functions in format.h are less the target of the<br>
verb and more the object/unit being affected.<br></blockquote><div><br>If you consider ast_format_interface as an implementation detail only relevant to developers of format.c and of actual formats/codecs, then the naming is not so loose.  To the consumer of format.h, ast_format_interface / the module providing it is of no concern.<br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Anyway:<br>
<br>
-- format.h --<br>
<br>
* ast_format_sdp_generate - rename to ast_format_sdp_generate_fmtp.<br>
The purpose of the function is only to generate the fmtp, and we may<br>
as well call that out. If we decide not to rename this, the API<br>
documentation should probably get cleaned up to reflect that the<br>
interface can generate whatever it wants for the SDP.<br></blockquote><div><br>Wouldn't ast_format_generate_sdp_fmtp more closely follow the suggested {verb}_{target} naming.  I have no idea's for if we should include "_fmtp" or not.<br>
 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- format_cache.h --<br>
<br>
None<br>
<br>
-- format_cap.h --<br>
<br>
* include format.h, as functions in this header return enum<br>
ast_format_cmp_res and use format structures extensively.<br>
<br>
* ast_format_cap_remove_bytype - rename to ast_format_cap_remove_by_type.<br>
<br>
* ast_getformatname_multiple - keep this function but deprecate it (it<br>
is used extensively and has been around for awhile). Instead, provide<br>
a renamed version: ast_format_cap_get_names(const struct<br>
ast_format_cap *cap, char *buf, size_t size).<br></blockquote><div><br>Any reason we don't use an ast_str for the new function?  Using ast_str would stop the creation of "char fmts[512]" and similar on the stack, and ensure we can make enough space for the buffer.  I know this would create more work but could be worth it.<br>
 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* ast_parse_allow_disallow - keep this function but deprecate it<br>
(again, used extensively). Provide a renamed version: int<br>
ast_format_cap_populate_from_config(struct ast_format_cap *, const<br>
char *list, int allowing). Note that this function currently returns<br>
the number of errors, but we rarely (if ever) check the return value.<br>
Instead, this should just return 0 or -1, and the caller should verify<br>
the return value.<br></blockquote><div><br>Agreed on the return value, the number of error's is irrelevant.  It either failed or it didn't.  Also for the case of failures, do we need to make this function atomic?  That is duplicate the originate format_cap, process with that, then replace the original only if we don't fail?  I know this will add overhead, but if any code is parsing repeatedly it should maybe be fixed?  I guess the added overhead could be an issue for realtime?<br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- format_compatibility.h --<br>
<br>
* ast_codec_pref* - These will all get removed if possible.<br>
format_pref is generally no longer needed. The biggest consumer of<br>
these is chan_iax2: if possible, it will use ast_format_cap<br>
structures.<br>
<br>
Note that format preference is handled by the ast_format_cap<br>
structure, and is the order in which the formats are added to the<br>
capabilities structure (which uses a VECTOR to maintain the formats).<br>
<br>
-- format_pref.h --<br>
<br>
* Remove entirely.<br>
<br>
-- codec.h --<br>
<br>
* struct ast_codec - remove bitfield specification from smooth. As the<br>
only bitfield in the structure, this is (a) unlikely to reduce the<br>
size of the structure and (b) will generally require more instructions<br>
to test given that it is a bitfield.<br></blockquote><div><br>Agreed that using bitfield here is silly.  Even if we do need more booleans later, I doubt we will need more than a couple.  We could use a byte for this field, give ourselves the ability to add 3 byte fields later without changing the ABI.  I would be fine with 'char smooth; char reserved[3];'.  We're not using a packed structure so this should result in 0 additional bytes of storage, since the compiler will reserve the 3 bytes if we don't (4 extra for 64-bit CPU's).  Also this seems like a good opportunity to reorder this structure - put pointers first, then int's to prevent wasting space.<br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* ast_codec_determine_length - rename to ast_codec_get_length<br>
<br>
* ast_codec_samples_count - move to frame.h. Rename to<br>
ast_frame_get_codec_samples.<br></blockquote><div><br>My vote would be for ast_frame_get_sample_count.  I agree that from the callers point of view they are not acting on a codec, they're acting on a frame.  ast_frame_get_samples sounds like it should return the buffer of samples.<br>
 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[1] <a href="https://issues.asterisk.org/jira/browse/ASTERISK-23715" target="_blank">https://issues.asterisk.org/jira/browse/ASTERISK-23715</a><br>
<span class=""><font color="#888888"><br>
--<br>
Matthew Jordan<br>
Digium, Inc. | Engineering Manager<br>
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA<br>
Check us out at: <a href="http://digium.com" target="_blank">http://digium.com</a> & <a href="http://asterisk.org" target="_blank">http://asterisk.org</a><br>
<br>
--<br>
_____________________________________________________________________<br>
-- Bandwidth and Colocation Provided by <a href="http://www.api-digital.com" target="_blank">http://www.api-digital.com</a> --<br>
<br>
asterisk-dev mailing list<br>
To UNSUBSCRIBE or update options visit:<br>
   <a href="http://lists.digium.com/mailman/listinfo/asterisk-dev" target="_blank">http://lists.digium.com/mailman/listinfo/asterisk-dev</a><br>
</font></span></blockquote></div><br></div></div>