Closed Bug 901560 Opened 11 years ago Closed 10 years ago

Stop adding ICE candidate lines for RTCP for DataChannels

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- disabled
firefox28 --- disabled
firefox29 --- fixed

People

(Reporter: ehugg, Assigned: ehugg)

References

Details

Attachments

(3 files, 8 obsolete files)

4.77 KB, patch
abr
: review+
Details | Diff | Splinter Review
5.44 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
5.45 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
Currently a datachannel-only SDP from createOffer looks something like this:

v=0 
o=Mozilla-SIPUA-25.0a1 3088 0 IN IP4 0.0.0.0 
s=SIP Call 
t=0 0 
a=ice-ufrag:2f402f73 
a=ice-pwd:b80230d8d5aa2eb8d57645c931f6967c 
a=fingerprint:sha-256 AF:EA:B9:46:BE:7C:E2:A7:8B:70:04:F3:5F:15:3B:EE:B4:7B:95:9A:41:60:2E:91:51:E2:53:45:06:7C:69:AE 
m=application 56930 DTLS/SCTP 5000 
c=IN IP4 216.243.14.31 
a=fmtp:5000 
protocol=webrtc-datachannel;streams=16 
a=sendrecv 
a=candidate:0 1 UDP 2113601791 216.243.14.31 56930 typ host 
a=candidate:0 2 UDP 2113601790 216.243.14.31 55083 typ host 

So there is a candidate line for RTCP which is not used in the DataChannel protocol.
It's more than just a matter of not adding the candidate lines. We shouldn't be even opening two components.
Thought I could get off easy by simply sending 1 for components into NrIceCtx::CreateStream(), but that didn't change the behavior.  

I found this:

    // XXX: components_ will be used eventually;  placate clang in the meantime.
    (void)components_;

In the constructor of NrIceMediaStream.  So I'm guessing it has to be implemented at that level first.
I don't think so. That's just whether we remember components_ or not.

Note that NrIceMediaStream::Create() takes components.

Could you be pulling the wrong media stream?
Yes, I jumped the gun on that comment. I'll put it under the debugger and find out why it didn't work.
In PeerConnectionMedia::Init we do three CreateStreams for audio/video/data not knowing what we're really going to negotiate.  So if I specify 1 component for data that only works if there is audio, video and data.  If there is just data, it seems to take the first ICE stream (named audio).  So, the order of m-lines is confused with the order audio/video/data that the ICE streams are created with.  

We should talk tomorrow about whether we should delve into a full fix for this or come up with a simpler partial fix.
So I created a hacky patch that removes the second component lines from the datachannel section, but it doesn't work unless I also change the component parameter in CreateStream to 1.  If you ask for two components and only connect 1 it appears to wait for the second to connect.

In light of that it seems that we'll need a separate list of ICE streams for media types and use the name of the ICE stream in the callback and match them up by media type instead of just level.
Assignee: nobody → ethanhugg
Attachment #794305 - Attachment is obsolete: true
Attachment #794312 - Attachment is obsolete: true
Attachment #794324 - Attachment is obsolete: true
Comment on attachment 794676 [details] [diff] [review]
Create ICE stream for datachannels with only one component


I changed ice_media_stream() to take a media type as well as level and attempted to do the CreateStream on the fly at first request.   This causes problems in a bunch of the code where we assume that ICE has already been started.

So instead, I still create them in PeerConnectionMedia::Init() but save them in a map by type.  Then when they are requested the first time they are put in a map by level.  This means the datachannel will get the ICE stream created with the name '/stream/data' regardless of which order the m= lines are.  So if I create the 'data' ICE stream with one component it will always be matched up with the 'DATA' media type.  This does not make the multiple-instances-of-same-media case any easier, however.

I also took out the last-minute -1s on level since it is now used as a key and not an index.
Attachment #794676 - Flags: review?(ekr)
If you had a mechanism to disable the second component after ICE gathering, would this still be needed?
Possibly, I would need to see if their media-type is known before we start getting candidate events so we could disable the second component in time.  When the disable mechanism arrives I can give it a try.
I think in order to use disable-component we will have to suppress the datachannel's second component candidate lines when creating the SDP.  This involves parsing the candidate lines for the dc and checking for the '2' which seems pretty hacky even for me.
Target Milestone: --- → mozilla26
Attachment #794676 - Flags: review?(ekr)
Attachment #794676 - Attachment is obsolete: true
Depends on: 909179
Depends on: 907353
Attachment #798013 - Attachment is obsolete: true
Comment on attachment 799731 [details] [diff] [review]
Interim fix of datachannel ICE components to be compatible with old and new versions


This is the interim version which still puts two candidate lines in the SDP but is compatible with both the current FF and the other patch on this bug which offers one candidate line only for datachannels.

The algorithm is that any connection gets the second candidate disabled except when it gives a o= line that identifies itself as an earlier version of Firefox.
Attachment #799731 - Flags: review?(ekr)
Comment on attachment 799786 [details] [diff] [review]
Datachannel no longer make second ICE component


This is the full version which will have only one candidate offered in the SDP.  We are disabling the second candidate at the first creation of the local SDP.
Attachment #799786 - Flags: review?(ekr)
I should mention that these patches require the patch from bug 907353 which in turn requires the patch from bug 909179.
Comment on attachment 799731 [details] [diff] [review]
Interim fix of datachannel ICE components to be compatible with old and new versions

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

This looks like Adam
Attachment #799731 - Flags: review?(ekr) → review?(adam)
Comment on attachment 799786 [details] [diff] [review]
Datachannel no longer make second ICE component

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

ABR
Attachment #799786 - Flags: review?(ekr) → review?(adam)
Comment on attachment 799731 [details] [diff] [review]
Interim fix of datachannel ICE components to be compatible with old and new versions

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

I'm not r+ or r- on this at the moment, since I'd like to better understand the rationale behind the "older than us" logic versus "older than or equal to a version known to exhibit special behavior" logic.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +121,5 @@
> +        /* This means we are talking to firefox, now read the major version */
> +        errno = 0;
> +        local_version = strtoul(MOZ_APP_UA_VERSION, &strtoul_end, 10);
> +        MOZ_ASSERT(errno == 0);
> +        MOZ_ASSERT(strtoul_end != MOZ_APP_UA_VERSION);

I don't think this is guaranteed to do what you want it to do. The problem is that the preprocessor is going to expand MOZ_APP_UA_VERSION in place to a string literal. While most compilers will optimize these two string literals to be exactly the same pointer, actual behavior is up to the implementation. cf. http://stackoverflow.com/questions/11144118/c-compare-two-string-literals

If you want to keep this assertion, you'll really want to throw a const char * on the stack, assign it to MOZ_APP_UA_VERSION, and use it for this purpose.

@@ +6887,5 @@
>        }
>  
> +      /* If this is Datachannel and we are talking to anything other
> +         than an older version of Firefox then disable the second component
> +         of the ICE stream */

I think we need more cautionary text in this comment about how this will go wrong if we leave this logic in place for more than one release version. It might also be worth considering whether we want to do a comparison against a known version number (e.g. "the other side is older than 26" rather than "the other side is older than we are").
Good catch on MOZ_APP_UA_VERSION, I obviously assumed that the compiler was required to store it once.

So I wrote this to check previous versions because I don't really have control over what version this lands in.  It could land in 26 or 27 or get uplifted to 25 for example.  It does need to be followed by the real patch one version later so I wasn't going to land anything until I get them both approved.

If you think I should change it and hard code version 26 then that would make the code simpler.  I would then of course have to land it this week.
Comment on attachment 799786 [details] [diff] [review]
Datachannel no longer make second ICE component

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

This looks good to me. The change in ice_media_stream.c seems superfluous, but I don't feel strongly enough about it to ask it to be removed.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5558,5 @@
>          if (media_enabled && ( media_cap->enabled || force_streams_enabled)) {
>              level = level + 1;  /* next level */
> +
> +            /* Datachannel does not use two ICE components */
> +            if (media_cap->type == SDP_MEDIA_APPLICATION) {

We might want to code this as:

> if (media_cap->type != SDP_MEDIA_AUDIO &&
>     media_cap->type != SDP_MEDIA_VIDEO) {

Simply to future-proof our behavior. For example, if we do end up with something of type SDP_MEDIA_DATA (or CONTROL or IMAGE) in the future, we don't want a second component for it.
Attachment #799786 - Flags: review?(adam) → review+
Attachment #799731 - Attachment is obsolete: true
Attachment #799731 - Flags: review?(adam)
Comment on attachment 801700 [details] [diff] [review]
Interim fix of datachannel ICE components to be compatible with old and new versions


Changed to assume it will push for FF26
Attachment #801700 - Flags: review?(adam)
Attachment #799786 - Attachment is obsolete: true
Comment on attachment 801704 [details] [diff] [review]
Datachannel no longer make second ICE component


Fixed nit from review.  Bringing forward r+ from abr.
Attachment #801704 - Flags: review+
Comment on attachment 801700 [details] [diff] [review]
Interim fix of datachannel ICE components to be compatible with old and new versions

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

Looks good to me.
Attachment #801700 - Flags: review?(adam) → review+
Interim fix pushed for FF26:

https://hg.mozilla.org/integration/mozilla-inbound/rev/727213fcc747
Whiteboard: [leave-open]
Found one nit in the interim patch.  I think that
      if (media->type == DATA &&
          !gsmsdp_requires_two_dc_components(sdp_p->dest_sdp)) {
should be:
      if (media->type == SDP_MEDIA_APPLICATION &&
          !gsmsdp_requires_two_dc_components(sdp_p->dest_sdp)) {
because the media->type is a sdp_media_e and not a cc_media_type_t

Fortunately they are the same value, but they will cause this warning:
gsm_sdp.c:6996:23: warning: comparison between ‘sdp_media_e’ and ‘enum <anonymous>’ [-Wenum-compare]

Also this code is part of the interim fix and will be removed in FF27 and replaced with the final change.
Attachment #801704 - Attachment is obsolete: true
Comment on attachment 803982 [details] [diff] [review]
Datachannel no longer make second ICE component


Updated main patch to include removal of interim patch.  Bringing forward r+ from abr.  I plan to push this one to FF27.
Attachment #803982 - Flags: review+
Main patch pushed to FF27

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd26d910cc2
Whiteboard: [leave-open]
Comment on attachment 824064 [details] [diff] [review]
Backout of compatibility-breaking datachannel ice component fix


This should backout the patch that was put into FF27 that made it incompatible with FF25.

created with hg backout -r 7cd26d910cc2
Attachment #824064 - Flags: review?(rjesup)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 824064 [details] [diff] [review]
Backout of compatibility-breaking datachannel ice component fix


r+ from jesup via IRC
Attachment #824064 - Flags: review?(rjesup) → review+
Backout pushed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9acc9c05346e
Whiteboard: [leave-open]
Comment on attachment 824064 [details] [diff] [review]
Backout of compatibility-breaking datachannel ice component fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
This bug - 901560 - It was decided that we needed more versions to be compatible with FF25.  This rolls FF27 back to the FF26 behavior.  We can re-add this patch perhaps in FF29.

User impact if declined:
FF27 Datachannels will remain incompatible with FF25 and earlier.

Testing completed (on m-c, etc.): 
Tested in FF26 this patch makes Aurora the same as FF26.

Risk to taking this patch (and alternatives if risky): 
FF26 could be broken somehow I suppose.  This should make Aurora and Nightly the same as FF26.

String or IDL/UUID changes made by this patch:
none.
Attachment #824064 - Flags: approval-mozilla-aurora?
Comment on attachment 824064 [details] [diff] [review]
Backout of compatibility-breaking datachannel ice component fix

Approving the backout to be in the same state as Fx26. Please add qawanted if there is any additional verification needed here.
Attachment #824064 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to Aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f2fa1bfc179

FF26,27 and 28 should now have the same behavior of offering two components and disabling the second if not talking to FF25 or earlier.
Keywords: qawanted
We tested from Win 7 64-bit to Win 7 64-bit (different networks) the below scenarios. 
On all Firefox versions we encountered small video hangs (the sound wasn't stopped) and a slight image focus for about one second (the image was zoomed in a bit and then recovered)

1. Fx 26b2- Fx 26b2: small hangs; focused image 
2. latest Aurora - latest Aurora: focused image
3. Fx 26b2 - Fx 25: small hangs, focused image
4. latest Aurora - Fx 25: the image is very blurry for few seconds during the connection; small hangs; focused image
5. latest Nightly - latest Nightly: small hangs, focused image, serious voice distortions - the sound wasn't clear (replaced by jerky sound)
6. latest Nightly - Fx 25: jerky sound
7. latest Nightly - latest Aurora: jerky sound
8. latest Nightly - Fx 26b2: focused image

Please let us know if further tests are needed.
Keywords: qawanted
QA Contact: petruta.rasa
That's great that we're new testing video quality between versions.  However, this bug deals specifically with connecting Datachannels.  Did your test make a Datachannel connection as well?  Thanks.
We tested 1 to 1 calls (https://apprtc.webrtc.org - ftp to ftp) to see if something regressed based on the changes. 
Could someone please provide a link to test and more detailed information about setting Data Channel connections? Thank you!
Apprtc doesn't use datachannels at all, so using it does not test this bug.  You need to use a datachannel test such as http://mozilla.github.com/webrtc-landing/data_test.html
Thanks for the link.

We tested it using latest Nightly, Aurora, beta (Fx 26b4) and Fx 25 and we didn't encounter any issues when sending text or files (large files can also be sent).

The following error appeared in data_test.html: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol."

Please let us know if further tests are needed.
https://hg.mozilla.org/mozilla-central/rev/3bf339937af5
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla26 → mozilla29
You need to log in before you can comment on or make changes to this bug.