873049 - Update data channel mochitests with no media streams involved to handle the check media cases

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the landing of the patches on bug 834835 we fixed the problem that streams were always combined. Sadly beside the actual patch the mochitests never got updated.

With my patch on bug 796894 I had to comment out the following check given that a datachannel test without any media wouldn't work. Given that is a bit more work and not only related to datachannels we want to fix this separately.

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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The following line also needs an update:
http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#420

Comment 2

[Jason Smith [:jsmith]]

I don't think this be taken care of a followup. See my comment I'm about to put on bug 796894. We shouldn't ever comment things out to reduce test coverage.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As said the amount of work for this change is more as I would like to do on bug 796894. As for now I will comment out the datachannel only test so it gets not run until this bug has been fixed.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> As said the amount of work for this change is more as I would like to do on
> bug 796894. As for now I will comment out the datachannel only test so it
> gets not run until this bug has been fixed.

Sorry, but I don't think that's good enough. I am not going to r+ that patch without this fixed.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jason Smith [:jsmith] from comment #4)
> Sorry, but I don't think that's good enough. I am not going to r+ that patch
> without this fixed.

Please try to be consistent across bugs. As of now your commenting style is rather confusing. See the comment you gave on the other bug:

> I don't think we should be commenting things out for all possible
> mochitests. That's going to cause us to lose coverage. I'd rather just
> disable this check for the specific test that this won't work with.

I don't see any problem when those three data channel tests which are involved here do not check for the media. Those never have requested some, so I don't see why this test would be that important as of now, and the checks could not be delivered as a follow-up.

Also this is a long outstanding issue and I really don't see why I should fix it on the data channel framework implementation bug. This is totally disconnected, sorry.

Comment 6

[Jason Smith [:jsmith]]

Fair enough, the fix I see in the tests makes this okay for a followup.

Comment 7

[Jason Smith [:jsmith]]

Reworking title to what's being fixed here.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Nils, if you find the time this bug most likely also necessary to get fixed.

Comment 9

[Nils Ohlmeier [:drno]]

Attachment: compare_constraints_with_tracks.patch

As the whole concept of streams does not very well fit the media constraints I changed it to the media tracks instead (since we have these calls available now).

Comment 10

[Nils Ohlmeier [:drno]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=fddf1bd900e3

Comment 11

[Nils Ohlmeier [:drno]]

Interesting try run results. I'm guessing the call to get(Audio|Video)Tracks() is timing sensitive (if no media has arrived yet it returns 0...). Lets see if re-run's will turn green. Then I'll have to search for solution to that problem...

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 8407848 [details] [diff] [review]
compare_constraints_with_tracks.patch

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

::: dom/media/tests/mochitest/pc.js
@@ +1300,5 @@
>          onSuccess();
>        }
>      }
>  
> +    if (this.constraints.length === 0) {

Why is this necessary? Shouldn't the _getAllUserMedia function already cause this behavior to happen?

@@ +1546,5 @@
>  
>    /**
> +   * Counts the amount of audio tracks in a given media constraint.
> +   *
> +   * @param constraint

Nit - constraints

@@ +1591,5 @@
> +   *        An array of streams (as returned by getLocalStreams()) to be
> +   *        examined.
> +   */
> +  countAudioTracksInStreams : function PCW_countAudioTracksInStreams(streams) {
> +    if (!streams || (streams.length == 0)) {

Nit - for any place using ==, use === instead

@@ +1597,5 @@
> +    }
> +    var audiotracks = 0;
> +    streams.forEach(function(st) {
> +        audiotracks += st.getAudioTracks().length;
> +      });

Nit - indentation

Comment 13

[Jason Smith [:jsmith]]

(In reply to Nils Ohlmeier [:drno] from comment #11)
> Interesting try run results. I'm guessing the call to
> get(Audio|Video)Tracks() is timing sensitive (if no media has arrived yet it
> returns 0...). Lets see if re-run's will turn green. Then I'll have to
> search for solution to that problem...

It's probably a regression. I remember we used to have bugs like this early in the webrtc development days where we were failing to populate the video/audio tracks before firing the onaddstream callback.

Comment 14

[Nils Ohlmeier [:drno]]

So from a few more try runs with more debug output (https://tbpl.mozilla.org/php/getParsedLog.php?id=38114060&tree=Try&full=1#error0) it is pretty clear that this is going on:

onaddstream gets called way after the PC_REMOTE_CHECK_MEDIA_TRACKS test step gets executed. Search for the "stream type is" debug statement in the above test output after the test failure.
Meanwhile the getRemoteStream() function seems to successfully return a MediaStream. But that stream is missing the expected tracks.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

So (at least until we change the underlying MediaStreamGraph implementation) we should wait to check for tracks until onaddstream fires.

Comment 16

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #15)
> So (at least until we change the underlying MediaStreamGraph implementation)
> we should wait to check for tracks until onaddstream fires.

Easier said then done: onaddstream will only fire if the stream will have tracks in it. This makes the overall logic quite a bit more complex....

Comment 17

[Nils Ohlmeier [:drno]]

Attachment: compare_constraints_with_tracks.patch

Addressed Jason's comments.
Added callback logic in case onaddstream hasn't fired yet.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=53fd87ed200a

Comment 18

[Nils Ohlmeier [:drno]]

Attachment: compare_constraints_with_tracks.patch

This patch checks if onaddstream is expected to fire, if it has fired, if not waits for it and has a timeout in case onaddstream should not fire.

Comment 19

[Nils Ohlmeier [:drno]]

Try run for the latest patch: https://tbpl.mozilla.org/?tree=Try&rev=e49339678a18

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 8414821 [details] [diff] [review]
compare_constraints_with_tracks.patch

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

::: dom/media/tests/mochitest/pc.js
@@ +1333,5 @@
>          onSuccess();
>        }
>      }
>  
> +    if (this.constraints.length === 0) {

Why is this needed? I think this would automatically happen with the recursive function that already exists.

@@ +1645,5 @@
> +  countVideoTracksInStreams: function PCW_countVideoTracksInStreams(streams) {
> +    if (!streams || (streams.length === 0)) {
> +      return 0;
> +    }
> +    var videotracks = 0;

Nit - use camel case here & above

@@ +1648,5 @@
> +    }
> +    var videotracks = 0;
> +    streams.forEach(function(st) {
> +        videotracks += st.getVideoTracks().length;
> +      });

Nit - is the indentation right here & above?

::: dom/media/tests/mochitest/templates.js
@@ +216,5 @@
>      function (test) {
> +      test.pcRemote.checkMediaTracks(test._local_constraints, function () {
> +        test.next();
> +      });
> +      //test.next();

Nit - remove this dead code here & above

Comment 21

[Nils Ohlmeier [:drno]]

(In reply to Jason Smith [:jsmith] from comment #20)
> ::: dom/media/tests/mochitest/pc.js
> @@ +1333,5 @@
> >          onSuccess();
> >        }
> >      }
> >  
> > +    if (this.constraints.length === 0) {
> 
> Why is this needed? I think this would automatically happen with the
> recursive function that already exists.

You are right that it is not needed. It only serves the purpose of making the logging easier to understand, because with the recursion it looks like gUM gets called for 0 streams, where now it is hopefully clear that gUM never gets called.

Comment 22

[Nils Ohlmeier [:drno]]

Attachment: compare_constraints_with_tracks.patch

Addressed NIT's from Jason's review.
Carrying forward r+ from jesup and jsmith.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfb9d3331ed

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1dfb9d3331ed