[asterisk-dev] [Code Review] Add mutestream manager action and MUTESTREAM() dialplan function

[Olle E Johansson]

> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 4
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line4>
> >
> >     Copyright (C) 2009, Olle E. Johansson

Fixed.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 31-38
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line31>
> >
> >     You probably don't need all of these included explicitly.

Just stolen from another module, so I'm innocent. Will experiment though :-)


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 53
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line53>
> >
> >     doxygen format :-)

Yup.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 71
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line71>
> >
> >     Please use ast_free()

Ok


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 90
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line90>
> >
> >     Take a look at the places where reviewboard has highlighted whitespace errors in red.

Fixed before review...


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 140-141
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line140>
> >
> >     Use ast_debug()

Done.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 161-170
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line161>
> >
> >     It looks like you have an extra level of indentation here.

Missed that. Fixed.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 163
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line163>
> >
> >     add a space after if

Absolutely.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 167-169
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line167>
> >
> >     ast_debug()

Fixed.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 180
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line180>
> >
> >     You must lock the channel around datastore operations.  That applies here and some other places in this module.

You got it.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 210-219
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line210>
> >
> >     The documentation will need to be in XML format.

Done.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 249-250
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line249>
> >
> >     There is a memory leak here.  The reference to the channel needs to be released.

Fixed


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, line 304
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line304>
> >
> >     AST_MODULE_LOAD_SUCCESS

ok.


> On 2009-08-31 12:57:30, Russell Bryant wrote:
> > /trunk/res/res_mutestream.c, lines 313-314
> > <https://reviewboard.asterisk.org/r/345/diff/1/?file=6309#file6309line313>
> >
> >     Why not?

Discussed on IRC: Now fixed. Will review modules that I copied logic from too ;-)


- Olle E


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


On 2009-08-31 12:47:29, Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/345/
> -----------------------------------------------------------
> 
> (Updated 2009-08-31 12:47:29)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When you need to be able to mute incoming or outgoing audio for a channel, this is the function you need.
> 
> I am a bit unsure of the unload functionality. What will happen if I unload an audiohook module when a channel is active? 
> 
> This work is inspired by app_jack.c and func_volume.c
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_mutestream.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/345/diff
> 
> 
> Testing
> -------
> 
> I've tested this in manager and with the dynamic features in features.conf.
> 
> 
> Thanks,
> 
> Olle E
> 
>