[asterisk-dev] Format branch naming conventions

[Matthew Jordan]

On Sun, Jun 22, 2014 at 12:22 PM, Corey Farrell <git at cfware.com> wrote:
> 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.

I thought about this some more, and I think you're right: it's not
just the naming of things that is different, but they very semantics.
Take ast_best_codec: the format returned by it is now ref counted.
Even if the function 'survived', usage of it is now different. If you
don't dispose of the reference returned to you, you'll now leak the
format object.

Breaking changes should be obvious. I'll replace functions where appropriate.

> 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.
>

I went back and forth on that in my head as well. I actually like
ast_format_generate_sdp_fmtp best, as it is explicit about what is
being created.

>>
>>
>> -- 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.

Agreed; I'll modify that to use an ast_str.

>>
>>
>> * 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?

On the face of it, I like the atomic nature: it implies that an error
in parsing is recoverable.

On the other hand, duplicating the format capability structure is
expensive. It makes configurations that list out their formats on
individual lines expensive - and as you pointed out, that would apply
especially poorly for realtime.

More generally, we've taken the position for some time now that a
configuration error should not allow Asterisk to proceed with that
configuration item. Doing so masks subtle and dangerous problems: for
example, take the following SIP peer:

[foo]
type=peer
disallow=all
allow=g722
allow=ylaw

Because I 'fat fingered' ulaw as ylaw, my peer will not support ulaw.
That's no good. If we swallow that configuration error by allowing the
format capability structure to only contain g722, the first few test
calls may even work with that peer - particularly if your test calls
are intraoffice. As soon as you go out to the PSTN, however, the calls
will fail. No good.

I'd much rather have Asterisk explicitly warn me that my configuration
was invalid so I could fix it, as opposed to going live with a broken
config and call issues. In that case, the correct thing to do when
ast_parse_allow_disallow fails is not to use the format capability
structure, but instead to fail module loading (if possible).

As an aside: I think ast_format_cap_add should be
ast_format_cap_append - it implies that the ordering of calling the
function matters.

>>
>> -- 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.

I'm always hesitant about using char as numeric values (yes, I know
they are; yes I know that this belies my non-C heritage). There's some
scuttle out there that it *may* be slower since a char is not likely
to be the native word size of the CPU, but then you get into cache
sizes and memory fetching and arghh...

I'll avoid premature optimization here for the most part - since the
structure is already very large, I don't feel bad about eliminating
the bit field. If we think we can get some bang for the buck with char
over int, then we can discuss that some more in the code review - but
without profiling, I'm not sure how much we'd gain.

>>
>> * 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.

Works for me!

I'll wait until the existing core changes get committed before going
with the overhaul.

-- 
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