[asterisk-dev] Format branch naming conventions

Sun Jun 22 12:22:46 CDT 2014

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.

On Sat, Jun 21, 2014 at 6:10 PM, Matthew Jordan <mjordan at digium.com> wrote:

> Hey all -
>
> I'm working on ASTERISK-23715 [1], which is a standardization/renaming
> of public functions/structures affected by the media format work (in
> the group/media_formats-reviewed branch). Two functions are called out
> on ASTERISK-23715, but I've gone through the other major public header
> files and made notes on other functions that I think could either use
> renaming, could be removed, or just other general tweaks.
>
> As an aside, I personally generally prefer the following nomenclature
> for naming:
>
> {namespace}_{object/unit}_{verb}_{target}
>
> For example, if I wanted a function that retrieved an identifier from
> a format, I would expect it to be named:
>
> ast_format_get_id
>
> Things get a bit more 'loose' when the item may exist in a particular
> header, but may not fit well as the target of the verb. For example,
> the format interface functions in format.h are less the target of the
> verb and more the object/unit being affected.
>

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.

> Anyway:
>
> -- format.h --
>
> * ast_format_sdp_generate - rename to ast_format_sdp_generate_fmtp.
> The purpose of the function is only to generate the fmtp, and we may
> as well call that out. If we decide not to rename this, the API
> documentation should probably get cleaned up to reflect that the
> interface can generate whatever it wants for the SDP.
>

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.

>
> -- format_cache.h --
>
> None
>
> -- format_cap.h --
>
> * include format.h, as functions in this header return enum
> ast_format_cmp_res and use format structures extensively.
>
> * ast_format_cap_remove_bytype - rename to ast_format_cap_remove_by_type.
>
> * ast_getformatname_multiple - keep this function but deprecate it (it
> is used extensively and has been around for awhile). Instead, provide
> a renamed version: ast_format_cap_get_names(const struct
> ast_format_cap *cap, char *buf, size_t size).
>

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.

>
> * ast_parse_allow_disallow - keep this function but deprecate it
> (again, used extensively). Provide a renamed version: int
> ast_format_cap_populate_from_config(struct ast_format_cap *, const
> char *list, int allowing). Note that this function currently returns
> the number of errors, but we rarely (if ever) check the return value.
> Instead, this should just return 0 or -1, and the caller should verify
> the return value.
>

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?

> -- format_compatibility.h --
>
> * ast_codec_pref* - These will all get removed if possible.
> format_pref is generally no longer needed. The biggest consumer of
> these is chan_iax2: if possible, it will use ast_format_cap
> structures.
>
> Note that format preference is handled by the ast_format_cap
> structure, and is the order in which the formats are added to the
> capabilities structure (which uses a VECTOR to maintain the formats).
>
> -- format_pref.h --
>
> * Remove entirely.
>
> -- codec.h --
>
> * struct ast_codec - remove bitfield specification from smooth. As the
> only bitfield in the structure, this is (a) unlikely to reduce the
> size of the structure and (b) will generally require more instructions
> to test given that it is a bitfield.
>

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.

> * ast_codec_determine_length - rename to ast_codec_get_length
>
> * ast_codec_samples_count - move to frame.h. Rename to
> ast_frame_get_codec_samples.
>

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.

>
> [1] https://issues.asterisk.org/jira/browse/ASTERISK-23715
>
> --
> Matthew Jordan
> Digium, Inc. | Engineering Manager
> 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
> Check us out at: http://digium.com & http://asterisk.org
>
> --
> _____________________________________________________________________
> -- 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140622/f6537ede/attachment.html>