[Asterisk-code-review] res rtp asterisk.c: Fix DTLS negotiation delays. (asterisk[11])

[Dade Brandon]

Dade Brandon has posted comments on this change.

Change subject: res_rtp_asterisk.c: Fix DTLS negotiation delays.
......................................................................


Patch Set 1:

Hi guys,

We have been testing this on our network live since last Thursday, and for some customers, it did not resolve the problem.

When we did a packet capture for those customers (on our server and on their side) the DTLS certificate would show as being sent twice from the server (once for RTP, once for RTCP) - and it would show as being received only once on the packet capture on their side.

So, presumably, some network device in between our server and theirs, is thinking that these nearly identical UDP packets are repeats - except obviously they aren't, and they aren't even to the same ports.

I wouldn't bring this up as a reason to re-evaluate the patch, except for that I found a solution.

First, I noticed that ast_rtp_remote_address_set is called twice on these streams, from different threads - the second call, is from ast_rtp_on_ice_complete when status == PJ_SUCCESS.

Thinking this is a router thinking that the second UDP packet was a repeat due to some terrible checksum somewhere, I thought that making the RTP cert go out in the first call to ast_rtp_remote_address_set, and the RTCP cert go out in the second, would potentially resolve the problem.  To my surprise, it did, except in about 1 out of 10 calls.

I moved both bio flushes directly in to ast_rtp_on_ice_complete, taking it out of ast_rtp_remote_address_set, and then noticed that the issue was resolved on 100% of calls (although with 0.02 more DTLS connection delay on average, which is inconsequential)

Somehow, by waiting until ice has finished connecting, the packet captures now show both packets being received.

I'm unsure of what would cause this that could be ice related, but it seems like the proper solution would be to move the dtls_srtp_check_pending to ast_rtp_on_ice_complete, which I can do, except I have two questions for the pros first:

1) DTLS may have this issue when ice is not being used -- I imagine I should keep the dtls_srtp_check_pending calls in ast_rtp_remote_address_set, for use only when ice is not being used - except, how can I detect if ice is not being used?
    1.a) does anybody disagree that this flush should be done in both places?
    1.b) is rtp->ice enough or do I need to check both rtp->ice and rtp->rtcp->ice ?   otherwise, how can this be detected?

2) If I'm putting these rtp/rtcp calls to dtls_srtp_check_pending in to two places, I feel that I should wrap the redundant code in to a new function.  Would dtls_srtp_flush_pending be an appropriate name?  It seems redundent with dtls_srtp_check_pending, however it would be adding locks, as well as checking if rtp->rtcp & if so, calling dtls_srtp_check_pending on rtp->rtcp->dtls.


3) If I put the call in to ast_rtp_on_ice_complete, it makes the most sense to do it as an 'else' to the if (rtp->dtls.dtls_setup != AST_RTP_DTLS_SETUP_PASSIVE) -- since for ACTIVE/ACTPASS, we send a handshake instead.   That's how I verified that the delay is gone in my test setups, so it definitely works.  However in ast_rtp_on_ice_complete, there is an if block "if (status == PJ_SUCCESS) { ... }".   I think that the code should be in there - if I move it in there, I believe I should also move the dtls_perform_handshake in to there, since currently that is being called even if status != PJ_SUCCESS.
    1.a)  Is there any disagreements to me moving this previously existing functionality, even though it is not directly related to ASTERISK-25614?

Thanks,


Dade at xencall.com

-- 
To view, visit https://gerrit.asterisk.org/1813
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I547f1be7e302dbf71f6553dd8cbc0657b1d0b908
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: No