Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite RTC ICE and DTLS transport tests with alternative dependencies #9424

Closed

Conversation

@soareschen
Copy link
Contributor

soareschen commented Feb 7, 2018

Fixes #9110 and #9111.

This PR rewrites the tests for RTCDtlsTransport and RTCIceTransport to have separate tests for getting the transports from both RTP sender/receiver and SCTP transport. The common parts of the assertions are factored out into separate functions so that they can be tested in multiple test cases.

I have tried to write the tests as dependency-neutral as possible, and test this specifically on Microsoft Edge 42.17083. However there are still some issues that cause the tests to still fail on Edge:

  • The RTCPeerConnection() constructor on Edge is non compliant and require minimally new RTCPeerConnection({ iceServers: [] }) to work. On my Windows machine none of the WPT WebRTC tests that use new RTCPeerConnection() would pass. I made local modification to all constructors so that the tests can at least run on Edge locally.

  • Edge only supports the legacy addStream() method and not addTrack() and addTransceiver(). I have specifically added a addTrackOrTransceiver() helper function that calls either addTransceiver(), addTrack(), or even addStream() so that we can continue testing the higher level features.

  • Edge does not implement getSenders() and getReceivers(). This makes it impossible to retrieve the RTCRtpSender and RTCRtpReceiver instances so that we can get the DTLS and ICE transport objects. The workaround of getting hold of an RTCRtpSender using addTrack() also does not work, because Edge only implements addStream() which returns only undefined.

So despite my best effort I am unable to tweak the tests to pass on any of the current browsers. :( (perhaps the methodology also doesn't feel right)

If there are internal builds of browsers that have implemented the new features, please do run the tests and feedback on the test result is appreciated.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 7, 2018

Build PASSED

Started: 2018-02-13 11:02:25
Finished: 2018-02-13 11:36:47

Failing Jobs

  • chrome:dev
  • safari:11.0
  • MicrosoftEdge:16.16299

View more information about this build on:

@nils-ohlmeier

This comment has been minimized.

Copy link
Contributor

nils-ohlmeier commented Feb 9, 2018

Firefox hasn't implemented any of the transport objects. So these will continue to fail in the foreseeable future.

@fippo
Copy link
Contributor

fippo left a comment

including adapter.js will get you an Edge RTCPeerConnection which supports addTrack and ice transports as well as dtls transports. It comes with its own quirks though (e.g. icetransport.statechange is still named icestatechange). More inline...


for (const sender of senders) {
dtlsTransports.add(sender.transport);
dtlsTransports.add(sender.rtcpTransport);

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

rtcp-mux is optional, this will be unset set quite often. Which afaics would throw an exception further down.

This comment has been minimized.

Copy link
@soareschen

soareschen Feb 13, 2018

Author Contributor

Hmm in the section for Set the RTCSessionDescription, the sender.transport and sender.rtcpTransport are always set together. Though I guess we can ignore the fact here and write test elsewhere for transport and rtcpTransport must both be set.

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

" instead let rtcpTransport equal transport." -- right, its not null. Still, this isn't relevant to check here.

This comment has been minimized.

Copy link
@soareschen

soareschen Feb 13, 2018

Author Contributor

I have changed the test to continue if it can get at least one RTCDtlsTransport from the sender/receiver.


for (const dtlsTransport of dtlsTransports) {
assert_true(dtlsTransport instanceof RTCDtlsTransport,
'Expect sctp.transport to be an RTCDtlsTransport');

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

no sctp in here

doSignalingHandshake
*/

// Wait for a connecting dtlsTransport to transist

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

transist into connected state -> become connected

'Expect pc to have at least one sender');

assert_greater_than(receivers.length, 0,
'Expect pc to have at least one receiver');

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

this is only going to hold in the transceiver model which is not what is tested here. Also senders and receivers share the same transport so taking both doesn't offer much value.
If you want to keep this requirement then I'd suggest adding a remote stream

This comment has been minimized.

Copy link
@soareschen

soareschen Feb 13, 2018

Author Contributor

I have changed the test to continue if it can get at least one RTCDtlsTransport from the senders/receivers.

const candidatePair1 = iceTransport1.getSelectedCandidatePair();
const candidatePair2 = iceTransport2.getSelectedCandidatePair();
return Promise.all(iceTransports.map(

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

this is much more complicated than waiting for pc1.iceConnectionState. Same goes for the dtls variant (which could look at connectionstate)

This comment has been minimized.

Copy link
@soareschen

soareschen Feb 13, 2018

Author Contributor

That's a good idea. I have changed it to wait for pc.connectionState to become connected.

'Expect candidate parameter to be non-null after data channels are connected');
function validateIceCandidateParameter(param) {
assert_not_equals(param, null,
'Expect candidate parameter to be non-null after data channels are connected');

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

this is no longer related to data channels. Here and elsewhere

@@ -114,77 +70,90 @@
assert_true(candidatePair.remote instanceof RTCIceCandidate,
'Expect candidatePair.remote to be instance of RTCIceCandidate');
validateCandidateParameter(iceTransport.getLocalParameters());
validateCandidateParameter(iceTransport.getRemoteParameters());
validateIceCandidateParameter(iceTransport.getLocalParameters());

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

there are the ice transport parameters, not candidate parameters.

} else if (typeof pc.addTrack === 'function') {
// Note: The mediaStream argument is optional in the spec, but we require
// it for now so that we can test older versions of Firefox that only
// implements addTrack with compulsary mediaStream argument.

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

compulsory

@@ -418,6 +418,31 @@ function getUserMediaTracksAndStreams(count, type = 'audio') {
});
}

// A dependency agnostic way of adding track depending
// on the supported method on a browser. Uses addTransceiver(track)
// first if available, otherwise use addTrack(track, mediaStream).

This comment has been minimized.

Copy link
@fippo

fippo Feb 13, 2018

Contributor

or falls back to the legacy addStream method (which has been removed from the spec).

soareschen added some commits Feb 13, 2018

const certs = dtlsTransport.getRemoteCertificates();
return createDtlsTransportsFromSenderReceiver(pc1, pc2)
.then(([dtlsTransports1, dtlsTransports2]) => {
const dtlsTransports = [...dtlsTransports1, ... dtlsTransports2];

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

Please use spaces around the ... operator.

@alvestrand
Copy link
Contributor

alvestrand left a comment

A number of things that I don't think are very readable. In particular, mixing state-changing (connecting PCs) and info-fetching (extracting the DTLS and ICE transports) makes the tests hard to read and maintain.

}
function validateConnectingDtlsTransport(dtlsTransport) {
if (dtlsTransport.state !== 'connected') {

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

If state is connected, this function will succeed. If that's intentional, add a comment.

@@ -418,6 +418,28 @@ function getUserMediaTracksAndStreams(count, type = 'audio') {
});
}

// A dependency agnostic way of adding track depending

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

English: "of adding track" => "to add a track"

@@ -418,6 +418,28 @@ function getUserMediaTracksAndStreams(count, type = 'audio') {
});
}

// A dependency agnostic way of adding track depending
// on the supported method on a browser. Uses addTransceiver(track)

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

Rewrite this to be more readable:

  • If addTransceiver(track) is available, use that.
  • If addtrack(track, mediastream) is available, use that.
  • Otherwise, use addStream(mediastream).
// support addTrack or addTransceiver.
return pc.addStream(mediaStream)
} else {
throw new Error('This test requires either addTrack or addTransceiver implemented');

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

Make the error correct - "This test requires one of addTrack, addTransceiver or addStream to be implemented". We can remove addStream from the error message at the same time as removing it from the code.

// is "connected", all RTCIceTransports and RTCDtlsTransports are in the
// "connected", "completed" or "closed" state and at least one of them is in the
// "connected" or "completed" state.
function waitConnectingPc(pc) {

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

This name is not obvious. Can we call it "waitForConnectedState"?

return dtlsTransports;
}

function getIceTransportFromDtlsTransport(dtlsTransport) {

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

This function is so small, I don't think it should be a function.

You can replace it with dtlsTransport.transport (dtlsTransport.iceTransport after the change agreed to in Lyon goes through).

return [...iceTransports];
}

// Create DTLS transports by creating data channels

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

English: "Create a pair of DTLS transports by creating a data channel from pc1 to pc2. Returns an array containing two RTCDtlsTransports, obtained from each PC via pc.sctp."

}

// Create DTLS transports by adding tracks and connecting
// two peer connections. Returns an array of two array of

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

As above, but also "Returns an array of two arrays of RTCDtlsTransports."

// and connecting two peer connections. Returns an
// array of two RTCDtlsTransports, which obtained
// from RTCSctpTransport in pc.sctp.
function createDtlsTransportsFromSctp(pc1, pc2) {

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

Suggest renaming this function "connectPeerConnectionsAndReturnDtlsTransports".

I'd really prefer to not have this function, but instead have "connectPeerConnnections" and the (existing) getDtlsTransportFromSctpTransport called from the test functions - merging these two disparate things makes the test harder to read.

});
}

function createIceTransportsFromSctp(pc1, pc2) {

This comment has been minimized.

Copy link
@alvestrand

alvestrand Nov 3, 2018

Contributor

These two functions also, I'd prefer that did not exist.

@soareschen

This comment has been minimized.

Copy link
Contributor Author

soareschen commented Nov 3, 2018

Closing this with #13886 as continuation.

@soareschen soareschen closed this Nov 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.