> On March 29, 2014, 8:12 p.m., Corey Farrell wrote: > > /trunk/funcs/func_periodic_hook.c, line 299 > > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line299> > > > > Can we detach the audio hook here? I know we escape the hook callback > > very early but it still feels wasteful to keep it enabled. > > > > Possibly remove state->disabled variable? > > Russell Bryant wrote: > As you said, the callback does just about nothing, so I don't think it > really hurts to keep it. > > But honestly, I would have done it this way if the audiohooks API was > built for dynamic adding/removing. It really isn't. No other code does > that. It all relies on channel destruction to tear it down.
I took a closer look at audiohook.c, I agree the removal isn't very pretty. > On March 29, 2014, 8:12 p.m., Corey Farrell wrote: > > /trunk/funcs/func_periodic_hook.c, line 206 > > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line206> > > > > state->last_hook is never pre-initialized to ast_tvnow(), so hooks > > would always trigger on first check after being created or re-enabled. > > Unless this behaviour is documented I would expect the first run after > > create or resume to be delayed state->interval seconds. > > Russell Bryant wrote: > Fair. I had originally intended for it to run immediately, but I think > it makes more sense to delay. > > jamuel wrote: > So is the implication then if I were to set the interval to 30 seconds > that the hook wouldn't run for 30 seconds and then run every 30 seconds? > That seems more counter intuitive. I'd think that it should run and then > repeat every interval seconds. > > Russell Bryant wrote: > Yes, that's right. I keep going back and forth but I think I agree with > you that running immediately makes the most sense. > > jamuel wrote: > It seems to make more sense especially when the interval is "really > long". When the interval is just a few seconds it doesn't really matter but > when it's say 30s or 60s (or even longer) then it seems weird to have to wait > that long for it to run the first time--especially in your original use case > of wanting to beep to indicate call recording . . . I'm not going to hold up this review for this finding, but I disagree. Please point me to one implementation of an interval where the first run is immediately. I can't think of any. Also with first run delayed by interval, if you want a beep immediately you can just play one. With no delay on first interval it's impossible to prevent the immediate beep. > On March 29, 2014, 8:12 p.m., Corey Farrell wrote: > > /trunk/funcs/func_periodic_hook.c, line 273 > > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line273> > > > > Need checks for !ast_strlen_zero on all args. Maybe also a verify the > > exten@context exists? > > Russell Bryant wrote: > I think this is all handled. For interval, sscanf() will just fail on an > empty string. For exten and context, defaults of "s" and "default" are > applied. > > Also, I don't think it should check for the exten to exist. If it > doesn't, errors should show up when it tries to run. Extensions can be > added/removed dynamically, so I think we should just assume the best here. same => n,Set(BEEP_ID=${PERIODIC_HOOK()}) I believe the above dialplan would crash. With no parameters all fields of args remain NULL. same => n,Set(BEEP_ID=${PERIODIC_HOOK(5)}) This will result in a value for args.interval, but all other parameters NULL. - Corey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3362/#review11441 ----------------------------------------------------------- On March 31, 2014, 12:11 p.m., Russell Bryant wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3362/ > ----------------------------------------------------------- > > (Updated March 31, 2014, 12:11 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > This commit introduces a new dialplan function, PERIODIC_HOOK(). > It allows you run to a dialplan hook on a channel periodically. The > original use case that inspired this was the ability to play a beep > periodically into a call being recorded. The implementation is much > more generic though and could be used for many other things. > > The implementation makes heavy use of existing Asterisk components. > It uses a combination of Local channels and ChanSpy() to run some > custom dialplan and inject any audio it generates into an active call. > > The other important bit of the implementation is how it figures out > when to trigger the beep playback. This implementation uses the > audiohook API, even though it's not actually touching the audio in any > way. It's a convenient way to get a callback and check if it's time > to kick off another beep. It would be nice if this was timer event > based instead of polling based, but unfortunately I don't see a way to > do it that won't interfere with other things. > > > Diffs > ----- > > /trunk/funcs/func_periodic_hook.c PRE-CREATION > /trunk/CHANGES 411572 > > Diff: https://reviewboard.asterisk.org/r/3362/diff/ > > > Testing > ------- > > Called the following extension (100@test), both letting it run all the way > through, as well as hanging up at various points in the middle. > > > [macro-beep] > > exten => s,1,Answer() > same => n,Verbose(1,Channel name: ${ARG1}) > same => n,Verbose(1,Hook ID: ${ARG2}) > same => n,Playback(beep) > > [test] > > exten => 100,1,Answer() > same => n,Set(BEEP_ID=${PERIODIC_HOOK(beep,5)}) > same => n,Wait(20) > same => n,Set(PERIODIC_HOOK(${BEEP_ID})=off) > same => n,Wait(20) > same => n,Set(PERIODIC_HOOK(${BEEP_ID})=on) > same => n,Wait(20) > same => n,Hangup() > > > Thanks, > > Russell Bryant > >
-- _____________________________________________________________________ -- 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
