On Wed, Apr 30, 2014 at 8:00 AM, Joshua Colp <jc...@digium.com> wrote:
> Matthew Jordan wrote: > >> >> On Wed, Apr 30, 2014 at 5:59 AM, Joshua Colp <jc...@digium.com >> <mailto:jc...@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
-- _____________________________________________________________________ -- 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