Closed Bug 873049 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: WebRTC, defect, P2)

24 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox22 --- affected
firefox23 --- affected
firefox24 --- affected

People

(Reporter: whimboo, Assigned: drno)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc-][tests][p=2])

Attachments

(1 file, 3 obsolete files)

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
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
No longer blocks: 834835
No longer depends on: 796894
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → DUPLICATE
(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.
Fair enough, the fix I see in the tests makes this okay for a followup.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Reworking title to what's being fixed here.
Blocks: dc-tests
Summary: Update peerconnection/datachannel mochitests for media checks of remote peer → Update data channel mochitest with no media streams involved to handle the check media cases
Summary: Update data channel mochitest with no media streams involved to handle the check media cases → Update data channel mochitests with no media streams involved to handle the check media cases
Priority: -- → P2
Depends on: 796894
Whiteboard: [WebRTC][tests] → [WebRTC][blocking-webrtc-][tests]
Nils, if you find the time this bug most likely also necessary to get fixed.
Assignee: hskupin → nobody
Status: REOPENED → NEW
Assignee: nobody → drno
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).
Attachment #8407848 - Flags: review?(rjesup)
Attachment #8407848 - Flags: review?(jsmith)
Attachment #8407848 - Flags: review?(rjesup) → review+
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 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
Attachment #8407848 - Flags: review?(jsmith) → review+
(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.
Depends on: 998546
Blocks: 834837
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.
Depends on: 998552
No longer depends on: 998546
So (at least until we change the underlying MediaStreamGraph implementation) we should wait to check for tracks until onaddstream fires.
No longer depends on: 998552
Whiteboard: [WebRTC][blocking-webrtc-][tests] → [WebRTC][blocking-webrtc-][tests][p=2]
Target Milestone: --- → mozilla32
QA Contact: jsmith → 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....
Addressed Jason's comments.
Added callback logic in case onaddstream hasn't fired yet.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=53fd87ed200a
Attachment #8407848 - Attachment is obsolete: true
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.
Attachment #8413980 - Attachment is obsolete: true
Attachment #8414821 - Flags: review?(rjesup)
Attachment #8414821 - Flags: review?(jsmith)
Try run for the latest patch: https://tbpl.mozilla.org/?tree=Try&rev=e49339678a18
Attachment #8414821 - Flags: review?(rjesup) → review+
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
Attachment #8414821 - Flags: review?(jsmith) → review+
(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.
Addressed NIT's from Jason's review.
Carrying forward r+ from jesup and jsmith.
Attachment #8414821 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1dfb9d3331ed
Status: NEW → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.