Closed Bug 962371 Opened 6 years ago Closed 6 years ago

De-prioritizes TURN TCP in case UDP works

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: drno, Assigned: bwc)

Details

Attachments

(2 files, 1 obsolete file)

It seems in the current implementation if a TURN server is used the choice between UDP and TCP being used just depends on fact which allocation succeeded faster.
This sometimes, depending on network conditions and speed of the TURN server, results in TURN TCP getting used even though UDP works as well.

TURN TCP should be de-prioritized in a way that it only gets chosen in case UDP does not work at all, so it only gets used as the last fall-back solution.
The simple fix here seems to be to add a transport check here and assign
different priorities for TCP and UDP.

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#345
Assigning to Byron, though I could do this if Byron is too busy.
Assignee: nobody → docfaraday
I thought the maybe even easier implementation for now, if we don't want to add transport priorities right now, could be to just have some internal flag which indicates that we have received for example at least one success response for UDP allocation request at the same server. If that flag is set when your TCP allocations complete you would wait a little bit more if the UDP allocations complete shortly after and use these instead.
But I'll leave up to implementer how to do it best.
We could go a couple of ways with this. We could either have a configurable priority value for any candidate that uses TCP, and I guess this would carry less weight than the candidate type (for example, if we end up implementing full ICE TCP, a TCP host candidate would be preferred over a relayed UDP candidate). But this seems somewhat less flexible than just adding a configurable NR_ICE_REG_PREF_TYPE_RELAYED_TCP (and, if we end up implementing full ICE TCP, we would add a new configurable for each candidate type for TCP).
(In reply to Byron Campen [:bwc] from comment #4)
> We could go a couple of ways with this. We could either have a configurable
> priority value for any candidate that uses TCP, and I guess this would carry
> less weight than the candidate type (for example, if we end up implementing
> full ICE TCP, a TCP host candidate would be preferred over a relayed UDP
> candidate). But this seems somewhat less flexible than just adding a
> configurable NR_ICE_REG_PREF_TYPE_RELAYED_TCP (and, if we end up
> implementing full ICE TCP, we would add a new configurable for each
> candidate type for TCP).

That seems pretty reasonable. In terms of the actual priorities, I'm guessing we'd bump relayed from 0 to 5, and put relayed_tcp down at 0?

An alternate approach that ends up being more flexible in the long-run is to have a TCP "penalty" that subtracts, say, 5 from the type preference if the transport is TCP (with a floor of zero, just in case). Combined with adjusting the relayed type priority from 0 to 5, this has the same effect as I describe above, and it means we don't need to touch this again for any future TCP adaptations.
(In reply to Adam Roach [:abr] from comment #5)
> (In reply to Byron Campen [:bwc] from comment #4)
> > We could go a couple of ways with this. We could either have a configurable
> > priority value for any candidate that uses TCP, and I guess this would carry
> > less weight than the candidate type (for example, if we end up implementing
> > full ICE TCP, a TCP host candidate would be preferred over a relayed UDP
> > candidate). But this seems somewhat less flexible than just adding a
> > configurable NR_ICE_REG_PREF_TYPE_RELAYED_TCP (and, if we end up
> > implementing full ICE TCP, we would add a new configurable for each
> > candidate type for TCP).
> 
> That seems pretty reasonable. In terms of the actual priorities, I'm
> guessing we'd bump relayed from 0 to 5, and put relayed_tcp down at 0?
> 
> An alternate approach that ends up being more flexible in the long-run is to
> have a TCP "penalty" that subtracts, say, 5 from the type preference if the
> transport is TCP (with a floor of zero, just in case). Combined with
> adjusting the relayed type priority from 0 to 5, this has the same effect as
> I describe above, and it means we don't need to touch this again for any
> future TCP adaptations.

Having a fixed penalty is simpler, but perhaps less flexible; it seems to me that stuff like UDP relayed versus TCP reflexive is a bit of a toss-up. If you wanted to prefer UDP relayed in that case, the penalty would have to be very large, which would then make it difficult to prefer TCP host over UDP reflexive (TCP host would probably not be too bad if we're talking about two endpoints on the same local network).
Comment on attachment 8363894 [details] [diff] [review]
Make priority for TCP relayed candidates lower than UDP relayed candidates.

Review of attachment 8363894 [details] [diff] [review]:
-----------------------------------------------------------------

Something I noticed that bugs me; we're not using the #defines when setting these config values in nricectx.cpp. This seems like a bad idea to me. Should I go ahead and fix them?
Attachment #8363894 - Flags: review?(adam)
Hey, let me know if this patch clears up the problem.
Flags: needinfo?(drno)
Comment on attachment 8363894 [details] [diff] [review]
Make priority for TCP relayed candidates lower than UDP relayed candidates.

Review of attachment 8363894 [details] [diff] [review]:
-----------------------------------------------------------------

I'd really like to see some unit tests on this bug before we close it. You can do them in a separate patch if you prefer.

r+ with nits.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +358,5 @@
>        case RELAYED:
> +        if(cand->base.protocol == IPPROTO_UDP) {
> +          if(r=NR_reg_get_uchar(NR_ICE_REG_PREF_TYPE_RELAYED,&type_preference))
> +            ABORT(r);
> +        } else if(cand->base.protocol == IPPROTO_TCP) {

nICEr style is to put a line break before the "else".

@@ +362,5 @@
> +        } else if(cand->base.protocol == IPPROTO_TCP) {
> +          if(r=NR_reg_get_uchar(NR_ICE_REG_PREF_TYPE_RELAYED_TCP,&type_preference))
> +            ABORT(r);
> +
> +        } else {

here, too.

::: media/mtransport/third_party/nICEr/src/ice/ice_reg.h
@@ +42,5 @@
>  #define NR_ICE_REG_PREF_TYPE_HOST       "ice.pref.type.host"
>  #define NR_ICE_REG_PREF_TYPE_RELAYED    "ice.pref.type.relayed"
>  #define NR_ICE_REG_PREF_TYPE_SRV_RFLX   "ice.pref.type.srv_rflx"
>  #define NR_ICE_REG_PREF_TYPE_PEER_RFLX  "ice.pref.type.peer_rflx"
>  

This extra space seems unnecesary.

@@ +43,5 @@
>  #define NR_ICE_REG_PREF_TYPE_RELAYED    "ice.pref.type.relayed"
>  #define NR_ICE_REG_PREF_TYPE_SRV_RFLX   "ice.pref.type.srv_rflx"
>  #define NR_ICE_REG_PREF_TYPE_PEER_RFLX  "ice.pref.type.peer_rflx"
>  
> +#define NR_ICE_REG_PREF_TYPE_RELAYED_TCP    "ice.pref.type.relayed.tcp"

In keeping with the tree-nature of the preference interface (and being sensitive to the fact that these changes need to be upstreamed, where the strings might be more exposed than they are in Firefox), let's use something more like "ice.pref.type.relayed_tcp"
Attachment #8363894 - Flags: review?(adam) → review+
(In reply to Byron Campen [:bwc] from comment #8)

> Something I noticed that bugs me; we're not using the #defines when setting
> these config values in nricectx.cpp. This seems like a bad idea to me.
> Should I go ahead and fix them?

Not without first consulting with EKR. I'm not sure why it's structured the way it is, but I suspect it was done rather purposefully.
EKR: see Byron's question in comment 8.
Flags: needinfo?(ekr)
Attachment #8363894 - Attachment is obsolete: true
Comment on attachment 8363971 [details] [diff] [review]
Make priority for TCP relayed candidates lower than UDP relayed candidates.

Review of attachment 8363971 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+
Attachment #8363971 - Flags: review+
(In reply to Adam Roach [:abr] from comment #11)
> Comment on attachment 8363894 [details] [diff] [review]
> Make priority for TCP relayed candidates lower than UDP relayed candidates.
> 
> Review of attachment 8363894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd really like to see some unit tests on this bug before we close it. You
> can do them in a separate patch if you prefer.

   Looking at the code, we are hard-coded to request TURN UDP allocations first. Without a test TURN server that can be configured to delay UDP allocations (or something that will eat the first TURN UDP request, maybe), this is going to be difficult to write a targeted test-case for. We can verify that the patch results in lower priority for the TURN TCP candidates though. Would that be sufficient?
(In reply to Adam Roach [:abr] from comment #12)
> (In reply to Byron Campen [:bwc] from comment #8)
> 
> > Something I noticed that bugs me; we're not using the #defines when setting
> > these config values in nricectx.cpp. This seems like a bad idea to me.
> > Should I go ahead and fix them?
> 
> Not without first consulting with EKR. I'm not sure why it's structured the
> way it is, but I suspect it was done rather purposefully.

I am not sure, tbh. It may be historical, or it may be some includefile
issue. If you try it and it works it's fine by me.
Flags: needinfo?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #17)
> (In reply to Adam Roach [:abr] from comment #12)
> > (In reply to Byron Campen [:bwc] from comment #8)
> > 
> > > Something I noticed that bugs me; we're not using the #defines when setting
> > > these config values in nricectx.cpp. This seems like a bad idea to me.
> > > Should I go ahead and fix them?
> > 
> > Not without first consulting with EKR. I'm not sure why it's structured the
> > way it is, but I suspect it was done rather purposefully.
> 
> I am not sure, tbh. It may be historical, or it may be some includefile
> issue. If you try it and it works it's fine by me.

I'll give it a go. Needinfoing myself as a reminder.
Flags: needinfo?(docfaraday)
Fix something that bugged me.
Attachment #8364512 - Flags: review?(adam)
Flags: needinfo?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #9)
> Hey, let me know if this patch clears up the problem.

I ran three test calls between MTV "Mozilla" and "Mozilla Guest" networks. In all three cases I saw a mix of UDP and TCP for the first few seconds, but then everything switches to UDP only.
So looks like it is working.
Flags: needinfo?(drno)
Comment on attachment 8364512 [details] [diff] [review]
Part 2. Remove some magic constants.

Review of attachment 8364512 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks good.
Attachment #8364512 - Flags: review?(adam) → review+
(In reply to Nils Ohlmeier [:drno] from comment #21)
> (In reply to Byron Campen [:bwc] from comment #9)
> > Hey, let me know if this patch clears up the problem.
> 
> I ran three test calls between MTV "Mozilla" and "Mozilla Guest" networks.
> In all three cases I saw a mix of UDP and TCP for the first few seconds, but
> then everything switches to UDP only.
> So looks like it is working.

I would have expected no UDP in most cases. Can you fwd the logs to Byron
so he can evaluate?
(In reply to Eric Rescorla (:ekr) from comment #23)
> I would have expected no UDP in most cases. Can you fwd the logs to Byron
> so he can evaluate?

Why would you expect no UDP?
I traced on the wire only on the "Mozilla" WiFi side so far, and that network supports UDP fine. So I want/expect it to use TURN UDP, rather then TURN TCP.
https://hg.mozilla.org/mozilla-central/rev/c4cd87e44213
https://hg.mozilla.org/mozilla-central/rev/5b888357a3b2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.