[asterisk-dev] A thread for format work

[Joshua Colp]

Matthew Jordan wrote:
>
> bridge_native_rtp up for review. One down, three hundred to go...
>
> But onward anyway. Next on the "list": format attributes!
>
> The new specification calls for setting attributes using
> ast_format_set_attr, which takes in a key/value pair along with the
> format to set the attribute on. While this seems like the appropriate
> place to store the attribute (since the attribute describes the format),
> there's some problems with setting the attribute after format creation.
> Namely:
>
> (1) A format object is immutable by convention. However, setting an
> attribute on a format is a mutable operation, which means we either (a)
> should set all attributes on format creation or (b) setting an attribute
> actually returns a copy of the format. Both of these approaches have
> drawbacks.

Indeed! Once the format leaves your scope or if you get it from 
somewhere else, no modifications for you!

> (2) Setting an attribute technically gets mapped back to a format
> attribute interface. This interface shouldn't require massive changes,
> but does need to be exposed through the format API as it generates the
> SDP attributes. How we expose that may need some tweaking, as ideally
> something using the format API won't care too much how it gets the SDP -
> just that it gets it.

It should get slimmed down quite a bit, and has already.

> :: Setting Attributes ::
>
> The biggest sticky point is clearly #1. I don't think we should lose the
> immutability convention - that feels bad, since there's a lot of benefit
> to treating a format as a read-only object (no locking!) - and a format
> shouldn't change very often. In order to maintain mutability, I'd remove
> the public format attribute setting function and instead do the following:

I disagree with this. Having the ability to set via name/value 
attributes could be extremely useful in the future. For example it would 
easily allow a module to exist which allows you to add to the cached 
formats of the system, with attributes. This is something that wasn't 
possible before. For simple things it would even take care of SDP.

> (1) We set attributes only as the result of parsing an SDP. Currently,
> this is the only time we set attributes. ast_format_sdp_parse would be
> modified to return an ast_format. Calling that function should result in
> either NULL (failure) or a new ast_format, which should replace the
> format passed to the function (which would be the burden of the channel
> driver). Most of the work of duplicating the existing format and safely
> putting formats onto it could happen within format.c. In that case,
> there isn't much need for an individual 'add this format' function.

Yeah, I think pushing this down to the format core is a good idea.

> (2) Since the only time we add attributes is during SDP generation,
> there isn't any need for a public API that sets formats. The variable
> parameters that can be passed to ast_format_set are - from what I can
> tell - never passed.

You know my thoughts above on this. ^_^

> (3) Producing the attributes is really just a matter of generating the
> SDP. This should be a public function in format.h that maps down to the
> attribute interface.

Yup, already there.

> :: Getting Attributes ::
>
> Since the format is read-only, it's safe to get the format attributes
> from a format object. There is one place where this is done using
> ast_format_isset. ast_format_get_value is never used. As the object is
> immutable, a public accessor for an attribute key could be used that
> returns a const char *, which would fulfil both functions.

The design currently gets away from the whole attribute key thing. 
Attributes are stored in whatever way the format interface has decided 
via an AO2 structure associated with the format itself. It needs a way 
to set/get that.

-- 
Joshua Colp
Digium, Inc. | Senior Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
Check us out at: www.digium.com & www.asterisk.org