[asterisk-dev] [Code Review] 3721: Masquerades: Framehook and Audiohook fixup

[rmudgett]

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



/branches/12/funcs/func_audiohookinherit.c
<https://reviewboard.asterisk.org/r/3721/#comment23008>

    Hmm.  I think for v12 the current documentation needs to be preserved with the deprecation notice added.  Users of v12.0.x to v12.4.x would still have AUDIOHOOK_INHERIT doing something and the wiki should still document the function.
    
    For v13/trunk the entire removal of the documentation as you have in this review is appropriate.



/branches/12/main/framehook.c
<https://reviewboard.asterisk.org/r/3721/#comment23009>

    The extra parentheses are not needed.  && has lower precedence than ==



/branches/12/res/res_fax.c
<https://reviewboard.asterisk.org/r/3721/#comment23010>

    A blank line should always separate variable declarations from code.  It makes them easier to spot.


- rmudgett


On July 15, 2014, 6:38 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3721/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 6:38 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 418716 
>   /branches/12/res/res_fax.c 418716 
>   /branches/12/main/framehook.c 418716 
>   /branches/12/main/channel.c 418716 
>   /branches/12/main/bridge_basic.c 418716 
>   /branches/12/main/audiohook.c 418716 
>   /branches/12/include/asterisk/res_fax.h 418716 
>   /branches/12/include/asterisk/framehook.h 418716 
>   /branches/12/include/asterisk/audiohook.h 418716 
>   /branches/12/funcs/func_audiohookinherit.c 418716 
>   /branches/12/bridges/bridge_native_rtp.c 418716 
>   /branches/12/CHANGES 418716 
> 
> 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
> 
>

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