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

Tue Mar 29 10:38:33 CDT 2011

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

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.

/trunk/apps/app_faxdetect.c
<https://reviewboard.asterisk.org/r/1116/#comment6721>

    A new application specifically for fax detection should not be necessary.

/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1116/#comment6722>

    This should not be necessary.  Fax detection when in gateway mode should occur in a framehook listening on the called channel.

/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1116/#comment6723>

    Not necessary, see above.

/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1116/#comment6724>

    Same.

/trunk/configs/res_fax.conf.sample
<https://reviewboard.asterisk.org/r/1116/#comment6725>

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

/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/1116/#comment6726>

    Not necessary, see above.

/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1116/#comment6727>

    These two functions are not necessary.  All of the T.38 handling should occur in a framehook attached to the called channel.

/trunk/include/asterisk/res_fax.h
<https://reviewboard.asterisk.org/r/1116/#comment6728>

    Only one framehook per gateway is needed.  Also in the manner that you use this data structure, it should be a struct instead of a union.

/trunk/include/asterisk/res_fax.h
<https://reviewboard.asterisk.org/r/1116/#comment6729>

    This pointer will not be necessary when the T.38 gateway feature is implemented entirely in a framehook.  The framehook will be responsible for creating the session, thus it will be able to store a pointer to it somewhere.

/trunk/include/asterisk/res_fax.h
<https://reviewboard.asterisk.org/r/1116/#comment6730>

    This lock won't be necessary either.  All of the framehook stuff should run in the same thread.  There may be some edge cases where this is not true, so it may be worth investigating.  IIRC channel operations are protected by a mutex anyway and all of the framehook operations occur in these critical sections.

/trunk/include/asterisk/res_fax.h
<https://reviewboard.asterisk.org/r/1116/#comment6731>

    This won't be necessary using a single framehook based implementation.

/trunk/include/asterisk/res_fax.h
<https://reviewboard.asterisk.org/r/1116/#comment6732>

    Gateway specific start, read/write, and destroy functions should not be necessary.  The fax driver can detect that a gateway session is being requested by checking the details structure for AST_FAX_TECH_GATEWAY.  The destruction of the gateway session can be handled in the normal destroy_session function.

    Using the existing read/write functions to convert frames in gateway mode is a little bit more tricky. My initial thought was to keep some sort of state in the fax driver when a frame is written and use the type of the written frame (AUDIO vs MODEM) to determine whether the next frames that are read should be T.38 frames or AUDIO frames. I think that is a workable idea.

    The other idea I had was to not use read() at all during gateway mode or to only use it to send frames in one direction and use a generator to send frames in the other direction.  The type of the frame passed to write() would determine which direction a converted frame should be sent in.

/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/1116/#comment6733>

    Unnecessary, see above.

/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/1116/#comment6734>

    same

/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/1116/#comment6735>

    This is not necessary. A framehook on the called channel will receive this frame eventually and can handle it there.

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6743>

    The final implementation will need to intercept frames in both directions.

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6738>

    The framehook should be set up when FAXOPT(t38gateway)=yes is set and destroyed if FAXOPT(t38gateway)=no is set.

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6737>

    This should be part of the framehook.

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6720>

    It is not necessary to reserve the fax session before creating it.  For this code, the session should probably be reserved when we first decide we may be doing some T.38 stuff on this channel.

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6736>

    same

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6742>

    This should not be in the configuration file. See above.

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6740>

    Same as below, match on "t38_gateway" and "gateway".

/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/1116/#comment6741>

    The framehook should be enabled or disabled here.

    Also this should accept "t38_gateway" and "gateway" as the option to set along with "t38gateway".

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.

- Matthew

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/20110329/9620a709/attachment-0001.htm>