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

[Kevin Fleming]

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

(Updated 2010-03-16 10:55:50.815121)


Review request for Asterisk Developers.


Changes
-------

Fixed whitespace in the frame.h changes.

Ensured that when a channel is already in 'negotiating' state when the FAX application is started, that none of the processes that send audio (CNG, CED and silence) will actually send any audio.


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.


Diffs (updated)
-----

  /trunk/channels/chan_sip.c 252754 
  /trunk/include/asterisk/frame.h 252754 
  /trunk/main/channel.c 252754 
  /trunk/res/res_fax.c 252754 

Diff: https://reviewboard.asterisk.org/r/556/diff


Testing
-------

Compile testing only so far.


Thanks,

Kevin