1115212 - Webrtc mochitests assume all remote tracks are delivered in a single onaddstream callback

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Byron Campen [:bwc]]

This baked-in assumption lives in dom/media/tests/mochitest/pc.js in |checkMediaTracks|:

http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#2359

This will not hold once the msid work lands.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug_1115212_onaddstream_counters.patch

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e87f642fa4a9

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8543411 [details] [diff] [review]
bug_1115212_onaddstream_counters.patch

As I keep getting 404's for trying to re-trigger test runs for the previous try run, here is another one:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a816c818bcf4

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8543411 [details] [diff] [review]
bug_1115212_onaddstream_counters.patch

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

Just minor objections.

::: dom/media/tests/mochitest/pc.js
@@ +1543,5 @@
>    this.mediaCheckers = [ ];
>  
>    this.dataChannels = [ ];
>  
> +  //this.onAddStreamFired = false;

Remove comment

@@ +2308,5 @@
>     *        The media constraints of the local and remote peer connection object
>     */
>    checkMediaTracks : function PCW_checkMediaTracks(constraintsRemote, onSuccess) {
>      var self = this;
> +    //var addStreamTimeout = null;

Remove commented out code here too

@@ +2360,5 @@
> +        self.onAddStreamAudioCounter + self.onAddStreamVideoCounter;
> +
> +      if (receivedRemoteTracks === expectedRemoteTracks) {
> +        _checkMediaTracks(constraintsRemote, onSuccess);
> +      } else if (receivedRemoteTracks >= expectedRemoteTracks) {

>

@@ +2361,5 @@
> +
> +      if (receivedRemoteTracks === expectedRemoteTracks) {
> +        _checkMediaTracks(constraintsRemote, onSuccess);
> +      } else if (receivedRemoteTracks >= expectedRemoteTracks) {
> +        ok(false, "Received more streams " + receivedRemoteTracks + 

Trailing ws

@@ +2376,5 @@
>        // we rely on the outer mochitest timeout to catch the case where
>        // onaddstream never fires
>        self.addStreamCallbacks.checkMediaTracks = function() {
> +        _compareReceivedAndExpectedTracks(constraintsRemote, onSuccess);
> +      }

Why not set this unconditionally, with a comment saying that we're doing this just in case we haven't received enough onaddstream callbacks yet?

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug_1115212_onaddstream_counters.patch

Removed commented code and additional white space.

Comment 5

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #3)
> @@ +2376,5 @@
> >        // we rely on the outer mochitest timeout to catch the case where
> >        // onaddstream never fires
> >        self.addStreamCallbacks.checkMediaTracks = function() {
> > +        _compareReceivedAndExpectedTracks(constraintsRemote, onSuccess);
> > +      }
> 
> Why not set this unconditionally, with a comment saying that we're doing
> this just in case we haven't received enough onaddstream callbacks yet?

This gets in fact called every time onaddstream fires. Sorry if that is not clear.
This registers _compareReceivedAndExpectedTracks() as one of the functions which gets called by the onaddstream callback handler.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8545502 [details] [diff] [review]
bug_1115212_onaddstream_counters.patch

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

One remaining nit.

::: dom/media/tests/mochitest/pc.js
@@ +2297,5 @@
> +        self.onAddStreamAudioCounter + self.onAddStreamVideoCounter;
> +
> +      if (receivedRemoteTracks === expectedRemoteTracks) {
> +        _checkMediaTracks(constraintsRemote, onSuccess);
> +      } else if (receivedRemoteTracks >= expectedRemoteTracks) {

Ok, my comment here was not very visible last time, this should be '>' right?

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: bug_1115212_onaddstream_counters.patch

Fixed remaining nit (if else condition).

Comment 8

[Nils Ohlmeier [:drno]]

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83e748ca3be8

Comment 9

[Nils Ohlmeier [:drno]]

Last try looked a little strange, lets try with a fresh one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3bc063026b8

Comment 10

[Byron Campen [:bwc]]

I'm seeing oranges on Mulet with this; maybe caused by a missing semicolon where we set the checkMediaTracks callback? I've kicked off a try run to test this hypothesis.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8546017 [details] [diff] [review]
bug_1115212_onaddstream_counters.patch

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

::: dom/media/tests/mochitest/pc.js
@@ +2307,5 @@
> +            receivedRemoteTracks + " vs " + expectedRemoteTracks + ")");
> +      }
> +    }
> +
> +    if (expectedRemoteTracks < (self.onAddStreamAudioCounter +

Here's our problem, this needs to be '>'

Comment 12

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #11)
> > +    if (expectedRemoteTracks < (self.onAddStreamAudioCounter +
> 
> Here's our problem, this needs to be '>'


Good catch. Thanks!

Comment 13

[Nils Ohlmeier [:drno]]

Attachment: bug_1115212_onaddstream_counters.patch

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd02330aa180

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8549193 [details] [diff] [review]
bug_1115212_onaddstream_counters.patch

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

::: dom/media/tests/mochitest/pc.js
@@ +2315,4 @@
>        // onaddstream never fires
>        self.addStreamCallbacks.checkMediaTracks = function() {
> +        _compareReceivedAndExpectedTracks(constraintsRemote, onSuccess);
> +      }

We want a semicolon here, probably.

Comment 15

[Nils Ohlmeier [:drno]]

Attachment: bug_1115212_onaddstream_counters.patch

Added the missing semicolon.
Carrying forward r+=bwc

Comment 16

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d93f0d813704

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d93f0d813704