[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/#review12527
-----------------------------------------------------------



/branches/12/CHANGES
<https://reviewboard.asterisk.org/r/3721/#comment22787>

    This should be in a new 12.5.0 section.



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

    12.5.0



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

    Asterisk 12 did the changes limiting when masquerades happen.
    
    Asterisk 12.5+ (because of this patch) made all audio hooks inheritable.



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

    Make this a one time only WARNING.
    
    "AUDIOHOOK_INHERIT is deprecated and now does nothing.\n"



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

    Since the function does nothing now, maybe should change to:
    "Audiohook inheritance placeholder function."



/branches/12/include/asterisk/framehook.h
<https://reviewboard.asterisk.org/r/3721/#comment22792>

    12.5.0



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

    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.
    



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

    Blank line not needed here.
    You are setting and then immediately testing.



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

    Either put the doxygen comment before the enum identifier and use /*!
    or use /*!<
    
    I think it is better to put the block comment before rather than as an EOL comment.



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

    Framehook is always freed by framehook_detach().  Framehook is used after free.


- rmudgett


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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140709/07c84968/attachment-0001.html>