[asterisk-dev] [Code Review] Improve handling of T.38 re-INVITEs that arrive before a T.38-capable application is executing on a channel.

[Mark Michelson]

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

Ship it!


I can't see anything wrong with this from a code standpoint. Since tests are passing, I'd say this is good to go.

- Mark


On 2010-03-18 15:20:02, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/556/
> -----------------------------------------------------------
> 
> (Updated 2010-03-18 15:20:02)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch addresses an issue found during working with end-users using res_fax. If an incoming call is answered in the dialplan, or jumps to the 'fax' extension due to reception of a CNG tone (with faxdetect enabled), and then the remote endpoint sends a T.38 re-INVITE, it is possible for the channel's T.38 state to be 'T38_STATE_NEGOTIATING' when the application starts up. Unfortunately, even if the application wants to use T.38, it can't respond to the peer's negotiation request, because the AST_CONTROL_T38_PARAMETERS control frame that chan_sip sent originally has been lost, and the application needs the content of that frame to be able to formulate a reply.
> 
> This patch adds a new 'request' type to AST_CONTROL_T38_PARAMETERS, AST_T38_REQUEST_PARMS. If the application sends this request, chan_sip will re-send the original control frame (with AST_T38_REQUEST_NEGOTIATE as the request type), and the application can respond as normal. If this occurs within the five second timeout in chan_sip, the automatic cancellation of the peer reinvite will be stopped, and the application will 'own' the negotiation process from that point onwards.
> 
> This also improves the code path in chan_sip to allow sip_indicate(), when called for AST_CONTROL_T38_PARAMETERS, to be able to return a non-zero response, which should have been in place before since the control frame *can* fail to be processed properly. It also modifies ast_indicate() to return whatever result the channel driver returned for this control frame, rather than converting all non-zero results into '-1'. Finally, the new request type intentionally returns a positive value, so that an application that sends AST_T38_REQUEST_PARMS can know for certain whether the channel driver accepted it and will be replying with a control frame of its own, or whether it was ignored (if the sip_indicate()/ast_indicate() path had properly supported failure responses before, this would not be necessary).
> 
> This patch also modifies res_fax to take advantage of the new request.
> 
> In addition, this patch makes sip_t38_abort() actually lock the private structure before doing its work... bad programmer, no donut.
> 
> This patch also enhances chan_sip's 'faxdetect' support to allow triggering on T.38 re-INVITEs received as well as CNG tone detection.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 253459 
>   /trunk/channels/sip/include/sip.h 253459 
>   /trunk/configs/sip.conf.sample 253459 
>   /trunk/include/asterisk/frame.h 253459 
>   /trunk/main/channel.c 253459 
>   /trunk/res/res_fax.c 253459 
> 
> Diff: https://reviewboard.asterisk.org/r/556/diff
> 
> 
> Testing
> -------
> 
> Tested with Zoiper Free; verified that without this patch Asterisk cannot receive a FAX from Zoiper because the T.38 invite arrives too early, but with this patch the FAX is received as expected. 'faxdetect' tested with Zoiper Free as well.
> 
> 
> Thanks,
> 
> Kevin
> 
>