> On July 9, 2014, 3:20 p.m., rmudgett wrote: > > /branches/12/funcs/func_audiohookinherit.c, line 309 > > <https://reviewboard.asterisk.org/r/3721/diff/1/?file=62408#file62408line309> > > > > Since the function does nothing now, maybe should change to: > > "Audiohook inheritance placeholder function."
changed. > On July 9, 2014, 3:20 p.m., rmudgett wrote: > > /branches/12/main/audiohook.c, line 604 > > <https://reviewboard.asterisk.org/r/3721/diff/1/?file=62412#file62412line604> > > > > This line is too long. You don't have to keep this long line as is > > just because you are moving it. This is the perfect time to fix the > > guideline violation. > > broke up the multiconditional if clause into two separate if clauses since we don't really like assignments as conditions anyway. if (!ast_channel_audiohooks(old_chan)) { return; } audiohook = find_audiohook_by_source(ast_channel_audiohooks(old_chan), source); if (!audiohook) { return; } > On July 9, 2014, 3:20 p.m., rmudgett wrote: > > /branches/12/main/audiohook.c, lines 616-618 > > <https://reviewboard.asterisk.org/r/3721/diff/1/?file=62412#file62412line616> > > > > Blank line not needed here. > > You are setting and then immediately testing. fixed. Plus I made sure I followed that convention in the above change. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3721/#review12527 ----------------------------------------------------------- On July 7, 2014, 5:21 p.m., Jonathan Rose wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3721/ > ----------------------------------------------------------- > > (Updated July 7, 2014, 5:21 p.m.) > > > Review request for Asterisk Developers, Joshua Colp, Matt Jordan, Mark > Michelson, and rmudgett. > > > Repository: Asterisk > > > Description > ------- > > It involves masquerades, so you know it's perilous. It involves FAX, so you > know it's maddening. > > The idea is fairly fairly simple at its core. During a masquerade, > audiohooks and framehooks will now be moved automatically (unless explicitly > set not to via a value in the framehook interface). Framehooks are a little > more complicated and some are simply not inheritable while others may require > some kind of fixup (none currently use this. The framehooks in res_fax ended > up getting a little too wierd to work well with the framehook fixup, so its > fixup had to be handled as part of the datastore fixup function.) > > It might be more appropriate to do this as an Asterisk 13 patch. I'm > uncertain right now. There are some crashes resolved by this (including > https://reviewboard.asterisk.org/r/3603/ which Corey Farrell is currently > trying to solve for 11 and maybe for earlier versions as well), but it's a > moderately big change and it likely has some potential to break things. > > > Diffs > ----- > > /branches/12/res/res_pjsip_refer.c 417692 > /branches/12/res/res_fax.c 417692 > /branches/12/main/framehook.c 417692 > /branches/12/main/channel.c 417692 > /branches/12/main/bridge_basic.c 417692 > /branches/12/main/audiohook.c 417692 > /branches/12/include/asterisk/res_fax.h 417692 > /branches/12/include/asterisk/framehook.h 417692 > /branches/12/include/asterisk/audiohook.h 417692 > /branches/12/funcs/func_audiohookinherit.c 417692 > /branches/12/bridges/bridge_native_rtp.c 417692 > /branches/12/CHANGES 417692 > > Diff: https://reviewboard.asterisk.org/r/3721/diff/ > > > Testing > ------- > > Tested masquerades on channels containing every different type of framehook. > Tested masquerades on channels containing every type of audiohook except for > app_jack (extended support and I haven't every used it, so I would need to > dig into its dependencies... plus none of the audiohooks so far have needed > any special attention anyway and app_jack looks less likely too than some of > the others). > > For everything with obvious audio impact (volume gain, mixmonitors, pitch > shift, chanspy etc), I also checked that audio kept going through as you > would expect it to. I also tested multiple audiohooks and multiple framehooks > for inheritance. Audiohooks were tested for multiple inheritance of the same > type (previously unavailable with the AUDIOHOOK_INHERIT function) > > > Thanks, > > Jonathan Rose > >
-- _____________________________________________________________________ -- 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
