This change has been marked as submitted.

Committed in revision 434704

Submitter:	Juergen Spies
Branch:
Bugs:	ASTERISK-24928
Depends On:
Blocks:

Reviewers
Groups:	asterisk-dev
People:
Repository:	Asterisk

Description:

Initialize "udptl->far_max_datagram" in t38_initialize_session() with a default value or a value provided
in pjsip.conf (t38_udptl_maxdatagram).
Without this far_max_datagram remains -1 if the remote endpoint does not provide the MediaAttribute T38FaxMaxDatagram
in it's SIP INVITE SDP. This will result in the INVITE being rejected.

Testing Done:

I have tested the patch with the following setup. The Asterisk is acting as relay.
The external T.38 gateway is at our ISP. The internal gateway is a Cisco SPA112.
The ISP’s gateway does not include T38FaxMaxDatagram in SDPs.

The test cases included:
- No t38_udptl_maxdatagram in pjsip.conf
  o Faxes in both directions with "normal" SDPs (all INVITEs created by Cisco),
  o Outbound fax w/o T38FaxMaxDatagram in SDP,
- t38_udptl_maxdatagram defined in pjsip.conf for both endpoints
  o Faxes in both directions with "normal" SDPs (all INVITEs created by Cisco).

All tests were completed successfully.

I think the t38_interpret_sdp function should be updated to only set the far max datagram if not explicitly configured. Right now it will set it again needlessly.

Juergen Spies April 6, 2015, 9:24 a.m. (April 6, 2015, 9:24 a.m.)

That is true if we can be sure that t38_initialize_session() has always been called before t38_interpret_sdp().
As far as I have seen, t38_interpret_sdp() is called from two locations:
1. defer_incoming_sdp_stream(): There this is clearly the case,
2. apply_negotiated_sdp_stream(): As far as I have seen it is called via a pointer to function from
   handle_negotiated_sdp_session_media(). Due to my limited knowledge of the inner dynamics of pjsip I was not able to
   ensure that t38_initialize_session() has really been called before.
Because of that I am not comfortable to remove this potentially redundant call from t38_interpret_sdp().
I would appreciate if someone w/ more experience with pjsip could look into this.

Joshua Colp April 6, 2015, 9:48 a.m. (April 6, 2015, 9:48 a.m.)

t38_initialize_session has to have been called before t38_interpret_sdp as it is what allocates the UDPTL session, which t38_interpret_sdp sets things on.

Diff
- added Diff r2 - Show changes

I want to give this a "Ship It!" but the "Testing Done" section is not filled in at all. What has been done to test this change?

Juergen Spies April 7, 2015, 5:56 p.m. (April 7, 2015, 5:56 p.m.)

I did test the 1st version of the patch even if I somehow missed that I should document it in the "Testing Done" field.
The 2nd version of the patch I will test on Friday. I will document the results then.

Testing Done

Changed from:

Changed to:

I have tested the patch with the following setup. The Asterisk is acting as relay.
The external T.38 gateway is at our ISP. The internal gateway is a Cisco SPA112.
The ISP’s gateway does not include T38FaxMaxDatagram in SDPs.

The test cases included:
- No t38_udptl_maxdatagram in pjsip.conf
  o Faxes in both directions with "normal" SDPs (all INVITEs created by Cisco),
  o Outbound fax w/o T38FaxMaxDatagram in SDP,
- t38_udptl_maxdatagram defined in pjsip.conf for both endpoints
  o Faxes in both directions with "normal" SDPs (all INVITEs created by Cisco).

All tests were completed successfully.

Ship It!

Ship It!

Ship It!

status changed from pending to submitted

Description:

Committed in revision 434704

You have a pending review.

Other reviews

Your comment

Review Board 1.7.16

This change has been marked as submitted.

res_pjsip_t38: Add missing initialization of udptl->far_max_datagram in t38_initialize_session()