| 03:04.06 | *** join/#asterisk-dev pabelanger (~pabelange@asterisk/contributor-and-bug-marshal/pabelanger) |
| 03:04.06 | *** mode/#asterisk-dev [+o pabelanger] by ChanServ |
| 03:08.24 | *** join/#asterisk-dev gtjoseph (~gtj0@unaffiliated/gtj) |
| 03:12.01 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 06:36.25 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 06:36.25 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 07:23.14 | *** join/#asterisk-dev tzafrir (~tzafrir@local.xorcom.com) |
| 07:23.14 | *** mode/#asterisk-dev [+o tzafrir] by ChanServ |
| 07:59.38 | *** join/#asterisk-dev danjenkins_ (~dan@cpc11-folk2-2-0-cust171.1-2.cable.virginm.net) |
| 12:35.17 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 14:08.08 | *** join/#asterisk-dev mjordan (~mjordan@75.76.55.191) |
| 14:08.10 | *** mode/#asterisk-dev [+o mjordan] by ChanServ |
| 14:44.50 | *** join/#asterisk-dev gtjoseph (~gtj0@unaffiliated/gtj) |
| 15:12.22 | file | pickles for mjordan! |
| 15:22.18 | *** join/#asterisk-dev danjenkins_ (~dan@cpc11-folk2-2-0-cust171.1-2.cable.virginm.net) |
| 15:28.32 | *** join/#asterisk-dev [TK]D-Fender (~chatzilla@64.235.216.2) |
| 15:29.54 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 15:29.54 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 15:35.10 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 15:45.30 | mjordan | all the pickles |
| 15:45.44 | mjordan | so, I do like the format_cache as a way of getting formats. It's fast. Fast is good. |
| 15:46.25 | mjordan | same thing with the extern pointers. For whatever reason - and this may be cognitive dissonance - I don't mind seeing an ast_format_opus there as a 'placeholder' that says hey - if it was loaded, here's a convenience pointer to it |
| 15:46.46 | file | why if it was loaded though? |
| 15:46.53 | mjordan | what was loaded? |
| 15:46.56 | file | opus |
| 15:46.57 | mjordan | format_opus? |
| 15:47.12 | mjordan | I'm fine with format_opus |
| 15:47.17 | file | not having a format_* or codec_* module loaded does not forbid it from working with passthrough |
| 15:47.21 | mjordan | I don't like it being in codec_builtin |
| 15:47.49 | mjordan | let's say someone creates codec_awesome next year |
| 15:48.08 | mjordan | Hopefully, if they write a codec_awesome/format_awesome, they can use our APIs to 'get the format', even if there is no cache |
| 15:48.13 | file | sure |
| 15:48.52 | mjordan | so... how are the builtins supposed to be replaced when the 'real thing' comes along? |
| 15:49.17 | file | what would the 'real thing' add that the builtins don't have already? |
| 15:49.21 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 15:49.28 | mjordan | well, let's use codec_opus again |
| 15:49.47 | mjordan | I can use the builtin version to make the cached format |
| 15:50.34 | mjordan | but that has (a) a fixed sample rate, and (b) doesn't provide the lookup table |
| 15:50.41 | mjordan | er, format_interface |
| 15:50.58 | file | ah, yes |
| 15:50.59 | mjordan | however, since I already created it, when a codec is loaded that provides the format_interface, how does it swap out with the builtin? |
| 15:51.13 | file | it can't currently |
| 15:51.46 | mjordan | and you really do want the format_cache to 'know' when a better version of something comes along |
| 15:52.09 | mjordan | otherwise, you'd end up in a discontinuity of 'if I request "opus" I get the good one, if I use ast_format_opus I get the limited one' |
| 15:53.06 | mjordan | I think ast_format_cache_add should be ast_format_cache_set, and it should just override whatever has been set for that format previously. The builtins can use it (and will) initially |
| 15:53.11 | file | that gets more complicated because you can't have multiple codec definitions registered |
| 15:53.19 | file | (they conflict) |
| 15:53.22 | mjordan | correct! There can be only one |
| 15:54.11 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 15:55.19 | *** join/#asterisk-dev [TK]D-Fender (~chatzilla@64.235.216.2) |
| 15:58.14 | mjordan | k. So. |
| 15:58.42 | mjordan | I'll at least start down the path of making the builtins create the cached formats. |
| 16:02.56 | file | mjordan, godspeed |
| 16:08.56 | mjordan | FYI: we have some weird structure to this stuff. ast_mod_format_register is defined in mod_format.h, but implemented in file.c? |
| 16:08.57 | mjordan | WTF? |
| 16:09.52 | mjordan | and oh good god |
| 16:10.02 | mjordan | every format that gets registered causes a stasis message to go out |
| 16:10.06 | mjordan | which then rebuilds the sound indexer |
| 16:10.21 | mjordan | I mean, I know why that is implemented that way but... yikes. |
| 16:10.50 | file | do you have a drink handy? |
| 16:19.30 | mjordan | not yet, but I'm going to need it |
| 16:20.58 | file | mjordan, oh! how was the party? |
| 16:22.47 | mjordan | good! Except for some dumbassery by a few guests |
| 16:24.16 | file | naturally |
| 16:26.33 | coreyfarrell | mjordan: ast_format_cache_set makes sense to me over add for exactly the reason you said |
| 16:26.34 | mjordan | file: for the codec builtins, were all the slin variants supposed to have the same name? |
| 16:26.47 | mjordan | coreyfarrell: cool, I'm working on that part now and will make that change then as well |
| 16:27.37 | file | mjordan, yes - sample rate is taken into account |
| 16:29.34 | mjordan | well... when they register with the format_cache, it will reject them |
| 16:29.42 | mjordan | as there is already a format with that name |
| 16:30.04 | file | format names different yes |
| 16:30.14 | file | slin, slin8, etc - same as how we know them now |
| 16:30.18 | file | er slin16 |
| 16:30.22 | coreyfarrell | something with ast_format_copy that I'm not a fan of: the standard usage is something->format = ast_format_copy(somethingelse->format). I think ast_format_set(dst**,src*) would be better so it can ao2_cleanup(*dst); |
| 16:32.19 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 16:32.19 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 16:32.26 | mjordan | kk |
| 16:32.47 | mjordan | IIRC, ast_format_copy is now just a bump on the ref count |
| 16:33.52 | file | mjordan, yeah he's referring to making it easier if you are replacing a format |
| 16:33.55 | coreyfarrell | correct. and in a lot of places it is the appropriate call - where you know the destination isn't already holding a ref. otherwise you have to ao2_cleanup |
| 16:34.32 | coreyfarrell | I'm not sure I'm suggesting that ast_format_copy go away, maybe call my suggested function ast_format_replace |
| 16:34.38 | mjordan | sure |
| 16:34.59 | file | that's fine |
| 16:35.29 | coreyfarrell | or maybe go even higher level, we have ao2_bump, ao2_cleanup, maybe ao2_replace would be nice? this idea seems like it would apply to the ao2 API |
| 16:36.59 | coreyfarrell | or I guess that would be lower level. guess I should finish my coffee before trying to think much more |
| 16:37.48 | file | your sense is being made. |
| 16:44.57 | file | sunrise, sunset, night and day |
| 16:48.06 | gtjoseph | coreyfarrell: what are you thinking about for ao2_replace? |
| 16:51.28 | coreyfarrell | gtjoseph: a macro: do { ao2_cleanup(dst); dst = ao2_ref(src); } while (0) |
| 16:51.55 | coreyfarrell | the real code would probably not reference dst twice, but you get the idea |
| 16:52.27 | mjordan | coreyfarrell: funny, I just wrote that in the format cache |
| 16:52.36 | gtjoseph | gotcha |
| 16:52.59 | mjordan | except I'm doing a void *tmp = dst; dst = ao2_bump(src); ao2_cleanup(tmp); to avoid any slip-ins between the assignments |
| 16:53.15 | coreyfarrell | mjordan: probably smart :) |
| 16:54.03 | coreyfarrell | in any case I've found myself wishing for that procedure to be in astobj2.h (for things other than media_formats as well) |
| 16:59.27 | gtjoseph | #define ao2_swap_objects(dst, src) void *tmp ; tmp = dst ; dst = ao2_bump(src) ; ao2_cleanup(tmp); |
| 16:59.38 | gtjoseph | I'll add it to my next patch. |
| 17:00.34 | coreyfarrell | gtjoseph: I don't like 'swap', that's not what it's doing |
| 17:00.45 | gtjoseph | ok |
| 17:00.47 | gtjoseph | replace |
| 17:01.16 | coreyfarrell | also ao2 is an abbreviation asterisk object, so I think "_objects" is redundant |
| 17:01.54 | gtjoseph | actually per mjordan's email yesterday it should be ao2_object_replace :) |
| 17:02.25 | mjordan | heh |
| 17:02.29 | mjordan | I'm not a zealot :-P |
| 17:02.44 | coreyfarrell | gtjoseph: ao2_{action} is the naming already used |
| 17:03.11 | mjordan | naming conventions are the hobgoblins of little minds |
| 17:03.54 | file | black magic |
| 17:04.33 | coreyfarrell | ao2_bump, ao2_cleanup, ao2_ref, etc.. even if ao2 follows a slightly different naming convention that I think is already set in stone, so we should keep using ao2_{action} for its new functions |
| 17:05.09 | mjordan | it's also common enough that there's no confusion about the naming |
| 17:05.29 | gtjoseph | ao2_replace it is. |
| 17:05.30 | mjordan | the whole "namespace_object_verb" nomenclature is particularly useful for APIs that are not used all over the codebase |
| 17:05.43 | mjordan | or when there may be confusion about what object they act upon |
| 17:06.05 | mjordan | hence my dislike for ast_parse_allow_disallow. If you're familiar enough with things you may know what that does |
| 17:06.15 | mjordan | but ... if you aren't, you have no idea what that does from the name |
| 17:06.17 | mjordan | parse it into what? |
| 17:06.19 | mjordan | JSON? |
| 17:06.37 | file | XML |
| 17:06.39 | coreyfarrell | without knowing better I would expect it to parse an ACL |
| 17:06.50 | mjordan | exactly |
| 17:07.20 | mjordan | ao2 is a library that you're going to use in just about every module. Its namespace defines that it acts upon ao2 objects. |
| 17:07.57 | mjordan | at least, that's how I justify the inconsistency to myself :-) |
| 17:09.58 | file | inhaling ice cream is not a good thing to do |
| 17:10.11 | coreyfarrell | I hate brain freeze |
| 17:13.33 | gtjoseph | you want ao2_replace to return anything? |
| 17:14.45 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 17:16.40 | coreyfarrell | gtjoseph: my idea would be no return, but maybe mjordan or file have a reason to think different |
| 17:16.59 | file | I don't really think returning anything would be useful. |
| 17:17.56 | mjordan | If it takes in the dst/src, I don't think it needs to return anything either. |
| 17:19.13 | mjordan | file: going back to opus for a second. As a builtin, it registers itself with the codec core |
| 17:19.17 | mjordan | say someone loads a codec_opus. |
| 17:19.41 | mjordan | (a) Does it need to provide an ast_codec? |
| 17:19.52 | mjordan | (b) If so, should the codec core be kind and replace the existing codec? |
| 17:20.02 | file | it doesn't need to, no |
| 17:20.14 | mjordan | (c) Should updating the format cache be the responsibility of the codec core, the module, or something else? |
| 17:20.26 | file | the module |
| 17:22.23 | mjordan | sighs |
| 17:23.18 | mjordan | what is supposed to provide the ast_format_interface? |
| 17:23.28 | mjordan | sorry, the sigh was realizing that ast_format_def is a thing |
| 17:24.24 | file | a module, or if need be something built in |
| 17:25.41 | file | I think practically a single 'entity' (module) should provide a codec definition, cached format definitions, and format interface all together |
| 17:25.51 | mjordan | right. |
| 17:26.02 | file | having stuff be mix and match across different ones is... hazardous |
| 17:26.03 | mjordan | I'm trying to figure out the order all the pieces should register and be put together |
| 17:26.13 | mjordan | it's a little haphazard |
| 17:26.37 | mjordan | but I'm down to only six linker errors! |
| 17:26.42 | mjordan | (granted, nothing is going to "work") |
| 17:27.22 | file | yeah, I think I found a problem in the translator (it's using the defined codec definition instead of retrieving the codecs from the core) |
| 17:28.02 | gtjoseph | here's your ao2_replace: http://pastebin.com/5dTfwmXM |
| 17:30.54 | coreyfarrell | do we need to be concerned about the 2 references to dst? I know this would not be normal, but ao2_replace(get_some_object()->destination, src); ? |
| 17:31.57 | gtjoseph | i think that still works |
| 17:32.05 | *** join/#asterisk-dev Dovid (~Dovid@72.1.252.10) |
| 17:32.09 | coreyfarrell | it would call get_some_object() twice :) |
| 17:32.25 | coreyfarrell | also what if we have: ao2_ref(src, +1); ao2_cleanup(dst); dst = src; |
| 17:32.45 | gtjoseph | true |
| 17:33.19 | file | and what about supporting if it's not set already? |
| 17:33.58 | coreyfarrell | file: you couldn't use this on an undefined dst |
| 17:35.49 | file | so, now depending on usage you've got to either be guaranteed that it's always set or do a check initially and either call this or ao2_bump |
| 17:35.57 | coreyfarrell | gtjoseph: you could use "typeof(dst) *__obj_ ## __LINE__ = &dst;" as your temporary variable to avoid calling dst twice |
| 17:36.27 | gtjoseph | true. |
| 17:36.32 | gtjoseph | will do |
| 17:36.54 | vlad_starkov | Question: Does anyone know how to use "audiofilter" utility to optimise your own voice prompts? See https://wiki.asterisk.org/wiki/display/AST/About+the+Sounds+Tools |
| 17:37.27 | file | walk the walk! #walkmore |
| 17:37.46 | coreyfarrell | file: you should know if a variable is undefined or not |
| 17:38.42 | coreyfarrell | that's something that should be known at compile time. if you have a variable that's not either ast_calloc'ed or NULL'ed then you just use dst = ao2_bump(src); |
| 17:39.34 | coreyfarrell | or if you are using dst in a loop then you would have the function declare void *dst = NULL; |
| 17:44.37 | vlad_starkov | malcolmd: Hi Malcolm, could you please point me how to can I use "audiofilter" utility to tune my custom audio prompts as you showed here https://wiki.asterisk.org/wiki/display/AST/About+the+Sounds+Tools ? Thanks |
| 17:45.03 | mjordan | being Sunday, malcolmd won't be on |
| 17:45.07 | mjordan | even if he is 'present' |
| 17:54.53 | file | coreyfarrell, sure... I meant whether it is non-NULL or not... cause as it is the above won't let you replace something that contains NULL |
| 17:56.32 | coreyfarrell | file: oh you mean if src is NULL. gtjoseph he's right we need to use ao2_bump(src) instead of ao2_ref |
| 17:56.56 | file | no, I mean if dst is NULL or heck even if src is NULL you can't do a replace |
| 17:57.53 | gtjoseph | i already check for both being non null and simply fail silently. |
| 17:58.00 | coreyfarrell | oh I didn't even see the if (src && dst) .. I don't think we want that |
| 17:58.13 | gtjoseph | ok, |
| 17:58.28 | coreyfarrell | gtjoseph: ao2_replace(some_non_null_object, NULL); should work |
| 17:58.58 | gtjoseph | in which case, dst just gets cleaned up? |
| 17:59.13 | *** join/#asterisk-dev Dovid (~Dovid@72.1.252.10) |
| 17:59.29 | coreyfarrell | yes.. better example: ao2_replace(some_non_null_ptr, some_possibly_null_ptr); |
| 17:59.44 | gtjoseph | ok |
| 18:00.47 | coreyfarrell | ao2_replace should work for situations where either variable is NULL, or if both are non-NULL. if both are null it should be a no-op by virtue of ao2_bump and ao2_cleanup ignoring NULL input |
| 18:00.56 | gtjoseph | getting big for a macro... how about ao2_replace(&some_non_null_ptr, some_possibly_null_ptr);? note the '&' and i make it an inline. |
| 18:02.33 | gtjoseph | coreyfarrell: nvermind the '&' how about it returns the new dst |
| 18:02.40 | coreyfarrell | what would be wrong with: do { ao2_bump(src); ao2_cleanup(dst); src = dst; } while(0) ? |
| 18:03.55 | coreyfarrell | ao2_replace(NULL, somepointer); would not be work. this is to allow: void *src = NULL; ao2_replace(src, somepointer); |
| 18:04.10 | coreyfarrell | or sorry I reversed the parameter there |
| 18:05.12 | gtjoseph | would still evaluate dstand src twice each |
| 18:06.15 | coreyfarrell | actual code should protect against that.. I cut out the copying cause this is IRC :) |
| 18:13.48 | coreyfarrell | gtjoseph: http://pastebin.com/7SSUPgBd is what I was thinking |
| 18:14.16 | gtjoseph | yep, that's what I've got |
| 18:17.13 | mjordan | and we have a binary! |
| 18:17.42 | coreyfarrell | mjordan: awesome!! how quickly does it crash? ;) |
| 18:18.08 | file | immediately, I'd wager |
| 18:18.17 | mjordan | has not run it yet |
| 18:18.58 | mjordan | I'm not even going to run it without gdb :-P |
| 18:19.17 | mjordan | yup, immediately :-) |
| 18:21.05 | coreyfarrell | mjordan: you want to post a review for your changes? that way I can merge your changes with mine and start thinking about the next step.. |
| 18:21.35 | mjordan | well, I've got a few blunders in here right now |
| 18:21.49 | mjordan | but as soon as I get through the obvious junk I will. Right now I badly locked the format_cache container |
| 18:21.57 | mjordan | soon as I get that cleaned up, I will |
| 18:22.46 | mjordan | should have something in about 30 min |
| 18:22.54 | coreyfarrell | sounds great |
| 18:22.56 | gtjoseph | coreyfarrell: here the final http://pastebin.com/6N3Vw1sH easier to do the null checks in this macro |
| 18:24.05 | mjordan | file: codec_dahdi will be an excellent use case of the 'codec swap' |
| 18:24.11 | mjordan | since it replaces builtin codecs with the HW variant |
| 18:24.13 | coreyfarrell | gtjoseph: a couple problems with that code |
| 18:25.03 | gtjoseph | other than checking dst twice? :) |
| 18:25.08 | file | mjordan, I wouldn't think codec_dahdi would need to do that... it's adding translators to the translation core, it shouldn't have to change the codec definitions - it's only using ulaw/g729 |
| 18:25.22 | mjordan | file: it currently has a static list of ast_codecs... |
| 18:26.12 | mjordan | file: also, since most ast_codec structures are not ao2 objects, should we either (a) assume that they are static, or (b) have an 'ao2 object' field in the structure that says this is malloc'd and ao2'd? |
| 18:26.21 | coreyfarrell | gtjoseph: also why not use ao2_cleanup? You're basically duplicating it |
| 18:26.23 | file | yeah, that's what I noticed earlier - when a translator registers it's supposed to use that to find a registered codec |
| 18:26.27 | mjordan | right now, ast_format_create attempts to treat it as an ao2 object |
| 18:26.33 | mjordan | ah |
| 18:26.53 | file | we could change that so every codec_* module does it itself to be more explicit/obvious |
| 18:27.03 | gtjoseph | coreyfarrell: __ao2_cleanup is a function call not a macro, saves a bit. |
| 18:27.18 | mjordan | file: that's probably an artifact as well of not having OBJ_KEY to perform lookups |
| 18:28.08 | file | mjordan, the end result is the same either way - the codec internally should be the one registered with the core, the only different is how a developer specifies which one to use for the source and destination codec in their translator |
| 18:28.36 | coreyfarrell | gtjoseph: right for use with RAII_VAR., understood. then the only other thing you need to fix coding guidelines (brackets and new-lines for the if's) |
| 18:29.07 | gtjoseph | that's a test to see if richard is on the ball. |
| 18:29.59 | mjordan | file: true. Back to the other question though: ast_format_create treats ast_codec as an ao2 object. Should it? |
| 18:30.05 | vlad_starkov | mjordan: Ok thanks! |
| 18:30.08 | file | mjordan, yes. |
| 18:30.16 | file | (imo at least) |
| 18:30.18 | mjordan | file: then builtins has to be redone |
| 18:30.29 | mjordan | you can't have static definitions of ast_codec |
| 18:30.38 | *** join/#asterisk-dev danjenkins_ (~dan@cpc11-folk2-2-0-cust171.1-2.cable.virginm.net) |
| 18:31.14 | mjordan | (Just got Asterisk running by ignoring that little problem) |
| 18:31.20 | file | ye of little faith |
| 18:31.31 | file | the act of registering creates an ao2 version |
| 18:32.25 | mjordan | heh |
| 18:32.29 | mjordan | then I've got to change my format_cache code |
| 18:32.46 | file | were you using it directly? |
| 18:32.49 | mjordan | yup! |
| 18:32.50 | mjordan | :-) |
| 18:32.54 | file | ah yup |
| 18:33.20 | mjordan | easy to fix |
| 18:33.26 | mjordan | (is glad he made a macro for it) |
| 18:33.30 | mjordan | as well |
| 18:33.53 | gtjoseph | mjordan: you ok if I just commit this little macro to 12 and trunk? No impact to anything existing. http://pastebin.com/jNFPLeuS |
| 18:33.56 | file | a fresh set of eyes is good since it shows where stuff could have been made more clear and explicit |
| 18:35.31 | mjordan | gtjoseph: works for me. And I like the ao2_ref usage, as that allows for better tracking of refs. Eventually, we may want a 'tag' version. |
| 18:35.38 | file | my mind is weird... it just imagined eyes floating in a bowl of punch |
| 18:35.40 | mjordan | just make the \since 12.4.0 |
| 18:35.51 | mjordan | file: creeeeeeepy. |
| 18:36.04 | gtjoseph | oh yeah. |
| 18:37.10 | mjordan | zomg: it started and shutdown without crashing |
| 18:37.49 | file | mjordan, now... can you configure an endpoint and have the configured formats displayed correctly? |
| 18:38.00 | mjordan | no |
| 18:38.04 | mjordan | (or I don't know) |
| 18:38.13 | mjordan | I explicitly disabled just about everything in menuselect |
| 18:38.16 | file | ah |
| 18:38.34 | mjordan | this just got the core off the ground |
| 18:38.42 | file | AHHHHHHHH Snoop Dogg do not want |
| 18:41.27 | mjordan | coreyfarrell: https://reviewboard.asterisk.org/r/3665/ |
| 18:44.20 | coreyfarrell | cool I'll check it out in a bit |
| 18:45.31 | coreyfarrell | so while I'm thinking of it, anyone want to give r3625 another look? I'm hoping to commit that soon. honestly I've been surprised by the low number of findings |
| 18:45.34 | mjordan | I'm going to keep on poking at the tying together of ast_format, ast_codec, ast_format_def, and those things |
| 18:46.10 | mjordan | coreyfarrell: I'm actually not surprised, a lot of things have a pretty well defined pattern for being converted over |
| 18:46.54 | mjordan | we're probably going to find all sorts of things once we start actually testing this stuff out |
| 18:47.30 | coreyfarrell | I have a file media_formats-since-r407438.patch that was open in a geany tab the entire time I was working on the code :) |
| 18:49.53 | mjordan | coreyfarrell: so the only 'finding' I have - and it's kind of a question, really - is do we really need publicly exposed pointers to the cached formats of ast_format_silk/celt? |
| 18:50.03 | mjordan | right now, there is not builtin for those two codecs |
| 18:50.15 | mjordan | s/not builtin/no builtin |
| 18:50.32 | mjordan | and they are used so rarely, I think having a module ask the format_cache explicitly for them is appropriate |
| 18:50.42 | coreyfarrell | checking my diff for why I did that |
| 18:51.03 | mjordan | rtp_engine, probably |
| 18:51.09 | coreyfarrell | channel.c I think |
| 18:51.20 | coreyfarrell | ast_best_codec I'm pretty sure is why I added those two |
| 18:51.28 | mjordan | k, I killed ast_best_codec with fire |
| 18:51.39 | mjordan | ast_rtp_engine_load_format however also explicitly calls those two out - although I'm not sure yet why |
| 18:52.28 | coreyfarrell | I can remove ast_format_celt/ast_format_silk, and if they are needed you can add them back in your review |
| 18:52.36 | mjordan | works for me |
| 18:52.56 | mjordan | ast_rtp_engine_load_format shouldn't really "need" them. It should be able to get the information from the format getting registered with the RTP engine |
| 18:53.10 | mjordan | we have a generic function that only supports two formats, and explicitly types them. Ew. |
| 18:53.48 | mjordan | coreyfarrell: if you want to keep it around to get Asterisk to compile that's fine, I've got place holders for them in my review |
| 18:53.54 | mjordan | but I'll work on getting them purged. |
| 18:54.21 | coreyfarrell | mjordan: I just removed them from format_cache.h in my working copy, I'm updating my review now. |
| 18:54.46 | mjordan | may take me a bit to extract it out of rtp_engine :-) |
| 18:55.18 | coreyfarrell | at this point given the large number of changes in my review I'd rather limit changes to "what I broke", get this committed. that way your review will be much smaller |
| 18:55.36 | mjordan | indeed |
| 18:55.39 | mjordan | I'll ship it! |
| 18:55.45 | mjordan | we know we have more to do in the core |
| 18:56.03 | mjordan | keeping this in bite sized chunks is the only way we'll get through it :-) |
| 18:56.11 | mjordan | goes off to take a break for a bit |
| 18:56.46 | file | does anyone else want to climb up a ladder and fix my downspout? |
| 18:56.55 | coreyfarrell | mjordan / file: given that this is not a mainline (or functioning) branch does the standard 24hour wait apply for this review? |
| 18:57.18 | file | coreyfarrell, I'll go with a no on that |
| 18:57.57 | coreyfarrell | awesome I'll get it committed in a few minutes so mjordan can repost his review to not include changes from almost 30 files |
| 19:02.10 | file | DESTROY AST_DATA |
| 19:03.29 | coreyfarrell | I'd happily submit a review to 'svn rm include/asterisk/data.h main/data.c' and make everything compile again :) |
| 19:05.18 | file | while I would gladly ship such a thing I'm sure mjordan would say something |
| 19:05.31 | coreyfarrell | gtjoseph: your commit included changes to Makefile that I didn't expect |
| 19:05.54 | file | gtjoseph, ditto - your tree was unclean |
| 19:19.29 | mjordan | I have never been so tempted to look the other way... |
| 19:43.50 | gtjoseph | rats i didn't notice. Just approve https://reviewboard.asterisk.org/r/3664/ and all will be well. |
| 19:44.35 | gtjoseph | or I could revert it. |
| 19:53.02 | gtjoseph | file: mjordan ^^ let me know. |
| 20:01.56 | mjordan | gtjoseph: I took a look and I can't recall what that flag is used for :-P |
| 20:02.07 | coreyfarrell | mjordan: when you have a chance do you want to rebase your review? |
| 20:03.29 | coreyfarrell | i'm building your branch right now and I'm going to do some smoke tests, but I'd rather do code review from RB instead of against my working copy |
| 20:04.17 | mjordan | sure, I can post it in a second. |
| 20:04.40 | mjordan | found a double lock of a wrlock in codec.c - recompiling now |
| 20:04.55 | mjordan | so there may be some debugging junk still in there :-) |
| 20:05.22 | coreyfarrell | that's fine.. it will still be a few files changed instead of a few dozen |
| 20:06.43 | mjordan | coreyfarrell: done |
| 20:06.45 | *** join/#asterisk-dev danjenkins_ (~dan@cpc11-folk2-2-0-cust171.1-2.cable.virginm.net) |
| 20:07.02 | gtjoseph | mjordan: you mean FORTIFY_SOURCE? it turns on things to help prevent buffer overflows, etc. |
| 20:07.32 | coreyfarrell | mjordan: I just noticed something crappy. media_formats-reviewed doesn't have your awesome REF_DEBUG code :( |
| 20:08.33 | mjordan | gtjoseph: Ah, okay. I'd say that's fine then. |
| 20:08.42 | mjordan | coreyfarrell: It is a bit behind... |
| 20:10.19 | coreyfarrell | I just patched my working copy with r412153 :) |
| 20:11.55 | gtjoseph | mjordan: Sorry about that. And to be clear FORTIFY_SOURCE is on by default, the patch only turns it OFF if DONT_OPTIMIZE is set otherwise the build will fail. |
| 20:23.09 | mjordan | gtjoseph: I'm guessing that patch may apply to 1.8+ |
| 20:23.42 | *** join/#asterisk-dev oej (~Adium@h87-96-134-129.dynamic.se.alltele.net) |
| 20:23.42 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 20:23.55 | gtjoseph | mjordan: i can do it for 1.8 and 11 |
| 20:25.02 | gtjoseph | mjordan: want me to add that macro back to 1.8 and 11 as well? |
| 20:33.19 | mjordan | was it removed from 1.8/11? |
| 20:33.27 | mjordan | or do we just not do it? |
| 20:33.45 | mjordan | file: so... slin. Right now codecs expect that to be matched based on the name + sample rate |
| 20:34.02 | mjordan | file: However, other places are not always very good about looking things up in that fashion |
| 20:34.07 | mjordan | for example, the translator core |
| 20:34.14 | gtjoseph | nope it's been the same since 1.8. The macro I was talking about is the ao2_replace just added. |
| 20:34.24 | mjordan | gtjoseph: ah. No, that should probably be 12+. |
| 20:34.33 | gtjoseph | thought so. |
| 20:34.50 | mjordan | file: There's going to be some other API changes if we require lookups by more than just name. Which direction do we want to go? |
| 20:35.29 | file | mjordan, can you clarify... more... |
| 20:35.41 | mjordan | file: codec_resample |
| 20:35.54 | mjordan | file: uses slin codecs and registers translation paths |
| 20:36.04 | mjordan | file: currently the name is the same, but the format is the different rate |
| 20:36.09 | file | sure |
| 20:36.21 | mjordan | file: the translation core currently only understands codecs by their ID, and attempts to map that |
| 20:36.24 | mjordan | (incorrectly right now) |
| 20:36.26 | file | yes |
| 20:36.37 | mjordan | while format_cache has to also map codecs to a hard coded format |
| 20:36.40 | mjordan | also by name |
| 20:36.43 | mjordan | the name is not sufficient |
| 20:36.56 | mjordan | so I had started down the path of making the name sufficient, i.e., slin16 instead of slin |
| 20:37.02 | file | where/ |
| 20:37.03 | file | ? |
| 20:37.07 | mjordan | but now I'm questioning that, since that's going back to how we were doing it |
| 20:37.09 | mjordan | in format_cache |
| 20:37.37 | mjordan | Before I go any further, want to make sure that we want to make whatever API changes are necessary to support matching codecs by multiple keys, i.e., name + sample_rate |
| 20:37.53 | mjordan | is working on getting the CLI commands working first |
| 20:37.55 | file | that support already exists... |
| 20:38.00 | file | (for codecs) |
| 20:38.02 | mjordan | k, it isn't everywhere it needs to be |
| 20:38.03 | mjordan | rigth |
| 20:38.06 | mjordan | codecs are fine |
| 20:38.14 | mjordan | formats + translation + everything that uses codecs doesn't assume that |
| 20:41.09 | file | I'm still lost |
| 20:42.24 | mjordan | ast_format_cache_set |
| 20:42.32 | mjordan | it takes in a name, and a format to make the cached format |
| 20:42.41 | file | yes. |
| 20:42.42 | mjordan | the name is insufficient to determine which cached format we should set |
| 20:43.35 | mjordan | I can determine it from the underlying codec using ast_format_get_sample_rate |
| 20:43.47 | file | define "we should set" |
| 20:44.09 | mjordan | you have hard pointers to nothing that are marked as extern |
| 20:44.15 | mjordan | *something* has to point them to something |
| 20:44.15 | file | oh, you mean those |
| 20:44.17 | mjordan | yes |
| 20:44.17 | *** join/#asterisk-dev jameswf (uid27319@gateway/web/irccloud.com/x-pjoefansbdfrqkoy) |
| 20:44.23 | file | you didn't say that :P |
| 20:44.43 | mjordan | sorry, that's in the review corey and I have been trading back and forth |
| 20:44.46 | mjordan | ast_format_cache_set is where that occurs |
| 20:45.11 | mjordan | Again, it's more just a "are we sure we really want to use two keys to find codecs". There's a lot of code that will be subtly broken by that change |
| 20:45.22 | mjordan | I'm just starting down into translate.c, and it's going to ripple in there |
| 20:45.32 | file | it has to be addressable by only name as well no matter what |
| 20:45.37 | mjordan | k |
| 20:45.44 | file | otherwise you are going to have to create yet another mapping |
| 20:45.49 | mjordan | so that means codec_resample, codec_builtins, and others should be updated |
| 20:46.10 | mjordan | i.e., we should not have multiple codecs that are named slin |
| 20:46.26 | mjordan | which also means searches in codec.c don't need to use sample_rate, as the name will always be a unique match |
| 20:46.46 | file | er |
| 20:48.02 | file | goes to read code |
| 20:48.26 | mjordan | In codec.c, we use OBJ_SEARCH_OBJECT (line 79 or so) and match codecs based on name, type, and sample rate |
| 20:48.31 | file | yes |
| 20:48.35 | mjordan | if name is a unique identifier, that's unnecessary |
| 20:48.39 | file | no |
| 20:48.41 | mjordan | why? |
| 20:48.45 | file | name is a unique identifier for FORMATS |
| 20:48.59 | file | name, sample rate, and type are a unique identifier for CODECS |
| 20:49.00 | mjordan | the fact that a codec name does not match the format name is going to be very problematic |
| 20:49.23 | mjordan | I can see where this code takes me, but I suspect it's going to get ugly in translate.c at some point |
| 20:49.38 | file | why is it going to be problematic? |
| 20:49.40 | mjordan | not to mention everywhere else that assumes it can use the codec name and the codec name only to determine something |
| 20:49.47 | mjordan | because translate.c *only* uses the codec name |
| 20:49.53 | mjordan | or at least, starts from there |
| 20:50.02 | file | for? |
| 20:50.02 | mjordan | I do not doubt that it is the only one |
| 20:50.07 | mjordan | to build the matrix |
| 20:50.26 | mjordan | and since things tend to use static codecs for lookup, everything has to make sure they provide those 3 values |
| 20:50.37 | mjordan | again, this is okay - just want to make sure we want to have this disconnect |
| 20:50.53 | mjordan | disconnect between formats (slin12) and codecs (slin + 12000 + audio) |
| 20:53.54 | file | oh I see why you are talking about this since you are in CLI command territory |
| 20:54.05 | mjordan | also: how is a format supposed to determine that its name is slin16 when the codec it builds from has a name of "slin"? |
| 20:54.44 | mjordan | ast_format_create takes in a codec, and maps its name directly to codec->name |
| 20:54.53 | mjordan | so... formats don't have slin16 either :-P |
| 20:56.11 | file | names are arbitrary and up to the developer or whatever creates them |
| 20:56.52 | file | oh, you mean the ACTUAL format structure? |
| 20:58.12 | file | yeah, the name is the codec - that function is better named ast_format_get_codec_name |
| 20:58.40 | mjordan | Right. But any user of the API has to care about the name of the format - that's used in several places |
| 20:59.58 | mjordan | Since that will now always return "slin" for slin16, or slin32, or whatever - there's less information being returned where it gets used. For logging purposes, meh. Not a huge loss. If someone tries to use that name for anything other than display purposes, it will be insufficient |
| 21:00.05 | mjordan | Which means we have a lot of code to triple check |
| 21:00.25 | coreyfarrell | how would that effect allow/disallow parsing? |
| 21:00.47 | mjordan | Don't know yet. Probably won't work. |
| 21:00.49 | file | allow/disallow pulls from the format cache based on the name given for its key |
| 21:00.57 | mjordan | but name won't work |
| 21:01.51 | coreyfarrell | I thought I remember seeing something like "slin:12000" or "slin:8000" ? |
| 21:01.54 | file | why not? when setting a format in the cache the name provided is used |
| 21:02.12 | mjordan | file: you are right, it can. However, trying to keep track of the difference between the two sucks a lot |
| 21:02.21 | mjordan | not a little, a lot :-P |
| 21:02.32 | file | what two? |
| 21:02.40 | mjordan | the difference between codec name and format name |
| 21:02.49 | mjordan | for example: codec_builtins is building out the format cache |
| 21:03.01 | mjordan | now, however, everything has to be carefully hardcoded, as you cannot use the codec name to set the name in the format cache |
| 21:03.09 | mjordan | because "slin" (for slin16) is not the right name |
| 21:03.33 | file | right. |
| 21:03.47 | mjordan | for example, this is now really, really wrong - https://reviewboard.asterisk.org/r/3665/diff/#7 |
| 21:04.04 | mjordan | having the name be different between codec/format just makes this a lot harder. |
| 21:04.18 | mjordan | it may be "the right way", but I'm not sure why we have to have two different schemas |
| 21:06.05 | gtjoseph | would a container that allows objects to be indexed by multiple keys help at all? |
| 21:07.09 | file | when doing that I thought of the name as the name of the codec - not of the codec at a specific sample rate, which allows further down the road to do examine things and see that two formats are the same codec but not the same sample rate |
| 21:07.23 | mjordan | not in this case - the codecs are already functioning with a multi-part key. The problem is there is a tight relationship between format and codec, and the two have different key schemas |
| 21:08.58 | mjordan | file: I'm not against that, as I think that does make more sense to uniquely identify the thing |
| 21:09.09 | mjordan | I'm trying to reconcile the two naming schemes however. |
| 21:09.26 | file | specifically naming formats I'd be for - allowing a format name AND a codec name |
| 21:09.30 | mjordan | right now, it feels like the danger of making a simple coding mistake is very high |
| 21:09.47 | mjordan | k |
| 21:09.50 | mjordan | I'll head down that path. |
| 21:10.12 | file | because trying to overload the codec name is just not going to work when attributes get involved |
| 21:10.53 | file | ie: I create an H264 format with X attributes and I create an H264 format with Y attributes - both go to the same codec name... if they have a format name as well then the name can include information about the attributes |
| 21:11.21 | file | then you can kill the wrapper structure in the cache |
| 21:11.27 | mjordan | yeah |
| 21:11.33 | mjordan | siren7/siren14: keep those as named? |
| 21:11.40 | mjordan | those are a bit ... 'odd' |
| 21:11.46 | file | they are different |
| 21:19.59 | mjordan | so the codec name should be siren7 and siren14 |
| 21:20.08 | file | yes |
| 21:24.49 | mjordan | follow up! Should a format interface be registered based on the format name, or the codec name? |
| 21:25.43 | file | hrm, I'd say codec name - they are specific to the codec |
| 21:27.04 | mjordan | k. So will slin:8000 and slin:16000 have different format interfaces? |
| 21:28.34 | file | they'll have none, but hrm... I'm thinking of stuff where they'd be applicable (h264, silk) |
| 21:29.22 | mjordan | if the sample rate means you must have a different interface, then you don't want to base it off the codec name |
| 21:29.34 | file | for h264 it has no sample rate so that's moot, but the question really is: would different modules have differing codec definitions with the same codec name and provide different format interfaces |
| 21:32.40 | file | I think in practice it's unlikely |
| 21:39.53 | *** join/#asterisk-dev danjenkins_ (~dan@cpc11-folk2-2-0-cust171.1-2.cable.virginm.net) |
| 22:00.48 | *** join/#asterisk-dev Dovid (~Dovid@ool-2f113d03.dyn.optonline.net) |
| 22:14.27 | file | I go away and a discussion does NOT break out |
| 22:14.30 | file | that's different |
| 22:17.46 | mjordan | I had an early dinner |
| 22:17.53 | mjordan | Hence, the lack of complaints :-P |
| 22:17.59 | mjordan | Just got done adding a name to the format structure |
| 22:18.00 | coreyfarrell | file: do you have any idea's on any way to start getting this stuff merged or rebased against a more recent /branches/12 |
| 22:18.03 | mjordan | so we shall seee |
| 22:18.20 | file | coreyfarrell, Just Do It(tm) |
| 22:18.31 | mjordan | coreyfarrell: I'm thinking the easiest will be to build a diff out of the various merges |
| 22:18.38 | mjordan | and then re-apply that to a new branch of 12/trunk |
| 22:18.53 | mjordan | In the end, this is probably going to end up being trunk only |
| 22:19.08 | file | mjordan, I am having my dinner NOW |
| 22:19.25 | file | it may involve pineapple |
| 22:20.07 | *** join/#asterisk-dev ivan` (~ivan@unaffiliated/ivan/x-000001) |
| 22:20.10 | coreyfarrell | I just ran asterisk then 'core stop gracefully' and it looks like a whole bunch of REF_DEBUG entry's are due to running against 12.0.0 |
| 22:20.51 | coreyfarrell | definitely some stuff caused by the media_formats branch, but not all (especially when I first tried without disabling res_pjsip) |
| 22:23.18 | mjordan | yeah, for now, res_pjsip is a bit of a sloppy room keeper |
| 22:23.21 | *** join/#asterisk-dev danjenkins_ (~dan@cpc11-folk2-2-0-cust171.1-2.cable.virginm.net) |
| 22:23.39 | mjordan | I'd keep it unloaded when attempting to make sure that the core is cleaning up |
| 22:24.08 | coreyfarrell | anyhow I would agree with trunk only for this. media_formats-reviewed already doesn't merge cleanly with branches/12, so might as well just go directly to trunk instead of having to deal with two large merges |
| 22:24.52 | coreyfarrell | not to mention 12 is security only in a few months |
| 22:25.56 | mjordan | yup |
| 22:25.58 | coreyfarrell | I know how much nobody is a fan of chan_sip, but I think it is the best target for 'early' testing. |
| 22:26.06 | mjordan | I'm fine with that |
| 22:27.45 | coreyfarrell | so I've never created an automerge or a team branch before, anything I need to know to create '/team/group/media_formats-reviewed-trunk'? |
| 22:28.18 | coreyfarrell | i'm assuming that's how we want to move forward, create a new branch off latest trunk, then start merging stuff from media_formats-reviewed? |
| 22:55.40 | *** join/#asterisk-dev mjordan (~mjordan@75.76.55.191) |
| 22:55.40 | *** mode/#asterisk-dev [+o mjordan] by ChanServ |
| 23:00.34 | file | LOCK ROBSTER! |
| 23:36.26 | mjordan | translation paths are working for everything but ... slin 8khz. Only when it is the src/dst. |
| 23:36.27 | mjordan | Bah. |
| 23:36.29 | mjordan | BAH. |
| 23:36.49 | file | mjordan, you get a gold star anyway |
| 23:37.33 | mjordan | I'm going to push this review only until translations show up |
| 23:40.14 | mjordan | HA |
| 23:40.28 | mjordan | slin 8000 is at position 0, and for some bloody reason we start counting at ... 1? |
| 23:41.29 | mjordan | I'm not even sure how any of this worked previously |
| 23:41.44 | mjordan | because I am way past what we modified with this patch |
| 23:42.01 | file | you mean the translation core in general? |
| 23:43.36 | file | ACHOO |
| 23:50.33 | mjordan | yes |
| 23:50.48 | mjordan | I at least want to have some confidence that the paths are set up |
| 23:50.50 | mjordan | I'm getting closer |
| 23:56.45 | *** join/#asterisk-dev snuff-work (~snuffy@210.8.167.13) |
| 23:56.45 | *** mode/#asterisk-dev [+o snuff-work] by ChanServ |