[asterisk-dev] [Code Review] 3424: mixmonitor: Add option to enable a periodic beep

[Corey Farrell]

> On April 8, 2014, 1:49 p.m., Corey Farrell wrote:
> > /trunk/main/app.c, line 3109
> > <https://reviewboard.asterisk.org/r/3424/diff/1/?file=57128#file57128line3109>
> >
> >     Can we create the context from func_periodic_hook?  My complaint is that for anyone not using func_periodic_hook this context is just wasted memory.
> >     
> >     Another way to deal with this would be for ast_app_get_beep_* to call init_beep_helper and track if we've created the extensions or not.
> >     
> >     In any case please ensure the internal dialplan is removed at shutdown.

I was actually hoping that the code in init_beep_helper would move to func_periodic_hook.  This is more of a nit, you've addressed my major concern which is to lifetime of the beep context.

On the topic of what code goes where, almost all the code could be moved to optional_api functions in func_periodic_hook.  One function to start and one to stop beeping on a channel.  This would have a few benefits:
* De-duplication of code to be used by res_monitor and app_mixmonitor.
* No code added to asterisk core.
* Beep dial-plan becomes a private implementation detail, and can use the func_periodic_hook context.


> On April 8, 2014, 1:49 p.m., Corey Farrell wrote:
> > /trunk/apps/app_mixmonitor.c, line 1075
> > <https://reviewboard.asterisk.org/r/3424/diff/1/?file=57126#file57126line1075>
> >
> >     I think this should be an error, and we should abort recording.  The main reason to use this flag will be for policy compliance reasons.  In that case it might be unacceptable or illegal to continue recording without beeps.

No LOG_ERROR?  Same comment for the other place we return -1.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3424/#review11515
-----------------------------------------------------------


On April 8, 2014, 7:49 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3424/
> -----------------------------------------------------------
> 
> (Updated April 8, 2014, 7:49 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add an option to enable a periodic beep to be played into a call if it
> is being recorded.  If enabled, it uses the PERIODIC_HOOK() function
> internally to play the 'beep' prompt into the call at a specified
> interval.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/app.c 412023 
>   /trunk/include/asterisk/app.h 412023 
>   /trunk/funcs/func_periodic_hook.c 412023 
>   /trunk/apps/app_mixmonitor.c 412023 
>   /trunk/CHANGES 412023 
> 
> Diff: https://reviewboard.asterisk.org/r/3424/diff/
> 
> 
> Testing
> -------
> 
> exten => 103,1,Answer()
>     same => n,MixMonitor(test.gsm,B(5))
>     same => n,MusicOnHold()
> 
> exten => 104,1,Answer()
>     same => n,MixMonitor(test.gsm,B)
>     same => n,MusicOnHold()
> 
> exten => 105,1,Answer()
>     same => n,MixMonitor(test.gsm,B(3))
>     same => n,StartMusicOnHold()
>     same => n,Wait(15)
>     same => n,StopMusicOnHold()
>     same => n,StopMixMonitor()
>     same => n,Wait(5)
>     same => n,Hangup()
> 
> 
> Thanks,
> 
> Russell Bryant
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140409/7869083c/attachment.html>