[asterisk-dev] A thread for format work

[Matthew Jordan]

On Wed, Apr 30, 2014 at 8:00 AM, Joshua Colp <jcolp at digium.com> wrote:

> Matthew Jordan wrote:
>
>>
>> On Wed, Apr 30, 2014 at 5:59 AM, Joshua Colp <jcolp at digium.com
>> <mailto:jcolp at digium.com>> wrote:
>>
>>     Matthew Jordan wrote:
>>
>>
>>
> <snip>
>
>
>>
>> If we allow attributes to be set after format creation, we have two
>> options:
>>
>> (1) We make formats mutable, and all the badness that entails.
>>
>> (2) Setting a format implicitly dupes the existing format, adds the new
>> attribute, then returns a new format.
>>
>> Going with option #2 (mutable == bad), I'm not sure how useful this is
>> as a public operation. When you get an SDP, the attributes will be
>> presented to you as a big ol' blob of text. You don't want to parse
>> those outside of something that is creating the format - otherwise
>> you'll end up with this incredibly bad code:
>>
>> format1 = ast_format_attribute_set(format, "foo", "bar");
>> format2 = ast_format_attribute_set(format1, "yackity", "schmackity");
>> format3 = ast_format_attribute_set(format2, "oh_nos", "the_pain");
>> ao2_cleanup(format2);
>> ao2_cleanup(format1)
>> ao2_cleanup(format);
>> format = format3;
>>
>
> I didn't mean replacing the SDP stuff with this. They are not equivalent.
> They are both different interfaces to the same thing, for different
> purposes.
>
>
>  Ew.
>>
>> This, on the other hand, is slightly cleaner:
>>
>> new_format = ast_format_sdp_parse(format, my_sdp_with_attributes);
>> ao2_cleanup(format);
>> format = new_format;
>>
>> And is how both chan_sip and res_pjsip_sdp_rtp operate.
>>
>> While we may some day want to add a public function that adds
>> attributes... YAGNI?
>>
>
> If you want to ditch it now, fine. It'll probably just end up getting
> resurrected in the future. :D
>
>
>
>>
>>         :: 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.
>>
>>
>>
>> How so? The current design calls for a public function that accepts a
>> const char *key and a const char *value. The key and value are always
>> going to be presented to the core as text (inbound SDP), and typically
>> will be generated as text (outgoing SDP). Yes, sometimes we may need to
>> interpret the values as integers, or floats, or something - but that
>> will typically be the domain of the resource modules. I'm not sure
>> storing them as heterogeneous types is all that advantageous.
>>
>
> Two words: Format comparison. The more complex codecs aren't just a string
> comparison to know if they are equal, they get turned into a different type
> and then a comparison done on that. The amount of times a format comparison
> is done throughout the lifetime will also be greater than that of just
> interpreting/generating, I wager. The question is... which one do we want
> to be fast?
>
>
True. OTOH, there is at least one place in the code (and maybe it should go
away? I don't know) outside of the format core that does check to see if an
attribute is set. We will have to 'deal with that', in some form or
fashion.

-- 
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140430/16916cc3/attachment.html>