[asterisk-dev] [Code Review] Fax Gateway Implementation T30<->T38

Matthew Nicholson reviewboard at asterisk.org
Thu Mar 31 11:20:08 CDT 2011 


> On 2011-03-29 10:38:33, Matthew Nicholson wrote:
> > This is a good start, but some modifications are necessary for this to fit in with the design described on the asterisk wiki (https://wiki.asterisk.org/wiki/display/AST/T.38+Gateway).  I have made some comments below.

https://wiki.asterisk.org/wiki/display/AST/T.38+Gateway


> On 2011-03-29 10:38:33, Matthew Nicholson wrote:
> > /trunk/apps/app_faxdetect.c, line 58
> > <https://reviewboard.asterisk.org/r/1116/diff/8/?file=15881#file15881line58>
> >
> >     A new application specifically for fax detection should not be necessary.
> 
> irroot wrote:
>     There is a application for this indeed esp with T.38
>     
>     previously when i wanted to receive a fax id answer / playtones / wait on the channel for the fax tone the channel driver will then divert it to fax exten [and disable ec] where i could check on the routing and even route it back to the same channel it would have gone to in the first place. once answered by the device diverting to the fax extension would leave the fax machine "negotiating" and not available for a fax so if im only using local fax app im fine here and it is not needed a perhaps WaitForFax is a more appropriate name for this app. this also removes the "i got a hangup" complaint where a fax would come in the user answers does not hear the initial tone and there is no call and assumes a call was dropped even if there was a fax in the inbox.
>     
>     this faxdetect is a enhancement on wait in that it runs DSP and will "queue" T.38 negotiation on detect making the switchover more painless.
>     
>     for outbound faxes this will have little application as the framehook bellow will cater for it.
>     
>     i find this most useful and have been using it for a long time.

For the use case you describe, you should just be able to turn on fax detection in chan_sip and do a Wait() on the channel in question before connecting the call.  chan_sip will detect the fax and send it to the fax exten.


> On 2011-03-29 10:38:33, Matthew Nicholson wrote:
> > /trunk/channels/chan_sip.c, lines 7049-7054
> > <https://reviewboard.asterisk.org/r/1116/diff/8/?file=15882#file15882line7049>
> >
> >     This should not be necessary.  Fax detection when in gateway mode should occur in a framehook listening on the called channel.
> 
> irroot wrote:
>     Doing it the t38gw framehook will hijack all fax detection from the channel drivers the idea was to have the T.38 capable drivers decide if they should send a switchover based on there settings. OOH323 already sends T.38 negotiate on fax detect.
>     
>     ill have to add DSP detection to the framehook that overrides the channel driver's im not sure if this is the right thing todo ??

The framehook runs after the channel has already had an opportunity to do any fax detection it will do.  The framehook running on the receiving channel will detect if that channel can support T.38 natively.  If it does, then the framehook won't actually need to do anything, if the channel does not support T.38/audio faxes natively then the framehook is there to translate the fax so that the receiving channel can understand it.


> On 2011-03-29 10:38:33, Matthew Nicholson wrote:
> > /trunk/configs/res_fax.conf.sample, lines 29-34
> > <https://reviewboard.asterisk.org/r/1116/diff/8/?file=15884#file15884line29>
> >
> >     This configuration file option is not necessary.  T.38 gateway support should be enabled on a per channel basis using a dialplan function (perhaps FAXOPT(t38gateway)=yes).
> 
> irroot wrote:
>     Indeed if i do not have the hook into the bridge and do it all in the frame hook then this is not required having the hook in the bridge will allow this to be a default option that if set to yes will be a implicit FAXOPT(t38gw....)=yes on calls relying on the switchover of the channel driver or the endpoint see T38SWITCHOVER in chan_sip see above / bellow.
>     
>     the hook for t38params in channel.c is related to this so to remove this all could be put into the framehook or leave it and have a global setting of running the gateway when needed without further dial plan intervention i favor this as it allows more flexibility but a coin toss.
>     
>     see channel.h/c below.

The framehook should be required to be explicitly attached to the receiving channel.  This can be done in the chan_sip and chan_dahdi configuration files using the "setvar" config option.  The framehook should only run on the receiving channel so running it unconditionally on every channel would not be ideal.


> On 2011-03-29 10:38:33, Matthew Nicholson wrote:
> > /trunk/main/channel.c, line 6933
> > <https://reviewboard.asterisk.org/r/1116/diff/8/?file=15888#file15888line6933>
> >
> >     Unnecessary, see above.
> 
> irroot wrote:
>     Flip a coin do we want a global enable option or must it be set in the dialplan only ?? this allows flexibility of configuration.

See above.  This should be enabled from the dialplan or configuration files.


> On 2011-03-29 10:38:33, Matthew Nicholson wrote:
> > /trunk/res/res_fax.c, line 2952
> > <https://reviewboard.asterisk.org/r/1116/diff/8/?file=15889#file15889line2952>
> >
> >     Same as below, match on "t38_gateway" and "gateway".
> 
> irroot wrote:
>     should we overload the function at this early stage ?? perhaps just use the generic FAXOPT(gateway) this will make support and documentation much simpler ??

I like FAXOPT(gateway) best as well, but I expect that people may use one of the other options I specified anyway.  It is not difficult to implement multiple options.  Only one would be used in the documentation.


On 2011-03-29 10:38:33, irroot wrote:
> > I have left out some comments on the implementation details, expecting them to change once everything is implemented in a framehook. Please review the asterisk wiki T.38 gateway document and my comments here and modify your implementation to match the design on the wiki. Thank you for your interest and effort surrounding this feature. If you have any questions, post them here, email me, or find me on IRC.
> 
> irroot wrote:
>     Thank you so much for the feedback ill be making some changes and implementing suggestions.
>     
>     the one thing that stands out is if we want to have a global option in res_fax.c that will enable gateway transparently or have it activated by the function exclusively.
>     
>     Im also not so sure about hijacking the fax detect in the channel drivers in the frame hook my FaxDetect/WaitForFax app is a channel independent dial plan option so is user implemented option the FaxDetect/WaitForFax app is used by me and is included in the hope it may be useful for others and is not critical to the gateway exclusively.

I commented on the global option above, the FaxDetect app is not necessary for channels that support fax detection.  For channels that don't support it I suppose it could be useful, but even that may be more useful as a framehook rather than being implemented as an application.


- Matthew


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


On 2011-03-16 08:40:59, irroot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1116/
> -----------------------------------------------------------
> 
> (Updated 2011-03-16 08:40:59)
> 
> 
> Review request for Asterisk Developers, Russell Bryant, dimas, Leif Madsen, and dafe_von_cetin.
> 
> 
> Summary
> -------
> 
> Hi there the patch that was going around circa 2008 to implement this in 1.4/1.6 app_fax has been moved to trunk [1.10]
> ive made some cleanups and moved it into res_fax res_fax_spandsp this is the framework and not production code
> unfortunately i have no means of testing it at the moment and require help.
> 
> i have cleaned the code up substantially it is related to R459
> 
> hope this is found useful and aids in the goal to get it in 1.10.
> 
> Adds application FaxGateway / FaxDetect
> Adds alternate bridge to Dial with new option.
> 
> 
> This addresses bug 13405.
>     https://issues.asterisk.org/view.php?id=13405
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_faxdetect.c PRE-CREATION 
>   /trunk/channels/chan_sip.c 310546 
>   /trunk/channels/sip/include/sip.h 310546 
>   /trunk/configs/res_fax.conf.sample 310546 
>   /trunk/configs/sip.conf.sample 310546 
>   /trunk/include/asterisk/channel.h 310546 
>   /trunk/include/asterisk/res_fax.h 310546 
>   /trunk/main/channel.c 310546 
>   /trunk/res/res_fax.c 310546 
>   /trunk/res/res_fax_spandsp.c 310546 
> 
> Diff: https://reviewboard.asterisk.org/r/1116/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> irroot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110331/8657eecd/attachment-0001.htm>

More information about the asterisk-dev mailing list