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

[Jonathan Rose]

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


Changes
-------

address rmudgett's findings


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 (updated)
-----

  /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/20140715/4f20981a/attachment.html>