When Bug 1081409 said this worked, it was only in debug builds (thanks to Bug 932845). But provided people wait for video.onloadedmetadata to fire before calling video.mozCaptureStreamUntilEnded(), then there's no reason this can't work reliably. STR: 1. In an opt-build, open URL and hit [Start] button. Expected result: - Video plays in small self-view and - after a few secs - in large remote-view. Actual result: - Video plays only in small self-view, with a "No SDP" error. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d42d379cb63a

I believe the verifySDP test in pc.js is incorrect in its count of m-lines to expect. Constraints on both sides need to be considered. E.g. If pc1 sends video with offerToReceiveVideo:false and pc2 sends nothing, then (per RFC3264) pc2's answer must still include a video m-line. pc2's verifySdp-call needs to look at pc1's constraints to know to expect that. The example is what Part 3 does, which is why I need it fixed.

Tests .mozCaptureStream video and audio over peerConnection. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=11a02972865b

Comment on attachment 8521126 [details] [diff] [review] Part 2: correct m-line test in verifySdp in test-harness Review of attachment 8521126 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with Nit's. I think ultimately we should have better solution then searching through the SDP text string, like suggested in bug 1045429. ::: dom/media/tests/mochitest/pc.js @@ +2376,5 @@ > trickleIceCallback(true); > } > //TODO: how can we check for absence/presence of m=application? > > //TODO: how to handle media contraints + offer options Looks like we can remove this comment now. @@ +2394,5 @@ > ok(desc.sdp.contains("a=rtcp-mux"), "RTCP Mux is offered in SDP"); > > } > > //TODO: how to handle media contraints + offer options Same here.

Comment on attachment 8521131 [details] [diff] [review] Part 3: test mozCaptureStream over peerConnection Review of attachment 8521131 [details] [diff] [review]: ----------------------------------------------------------------- You just created (unknowingly?) the first sendonly test case. That makes me wonder if should either rename this test case or create a separate test case specifically for sendonly only. Assuming you are going to address a few of the comments: looks good to me. ::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html @@ +18,5 @@ > + title: "Captured video-only over peer connection", > + visible: true > + }); > + > + var domloaded = new Promise(r => addEventListener("DOMContentLoaded", e => r())); Can we camel case domLoaded to make it more readable? @@ +21,5 @@ > + > + var domloaded = new Promise(r => addEventListener("DOMContentLoaded", e => r())); > + var test; > + var stream; > + var waituntil = func => new Promise(resolve => { waitUntil would also look better to me. @@ +35,5 @@ > + .then(() => waituntil(() => v1.videoWidth > 0)) // TODO: Bug 1096723 > + .then(function() { > + stream = v1.mozCaptureStreamUntilEnded(); > + info(stream.getTracks().length + " track(s)"); > + stream.getTracks().forEach(tr => test.pcLocal._pc.addTrack(tr, stream)); Maybe you should add an ok(stream.getTracks().length >= 1, "...") before this to prevent that this loop does nothing and you are left with just the info message above. Probably you could replace the info() with an ok(). @@ +36,5 @@ > + .then(function() { > + stream = v1.mozCaptureStreamUntilEnded(); > + info(stream.getTracks().length + " track(s)"); > + stream.getTracks().forEach(tr => test.pcLocal._pc.addTrack(tr, stream)); > + test.pcLocal.constraints = [{ video: true, audio:true }]; // fool tests Is this still needed even with patch 2? If so shouldn't we fix it or at least file a bug for it for later? @@ +41,5 @@ > + test.next(); > + }) > + .catch(function(reason) { > + ok(false, "unexpected failure: " + reason); > + SimpleTest.finish(); On a local manual test run from command line this works, on build/test servers this unfortunately not necessarily exits the test early (I'm assuming that's what you want here).

(In reply to Nils Ohlmeier [:drno] from comment #4) > You just created (unknowingly?) the first sendonly test case. Yeah, I assumed from patch 2. I was surprised to learn we weren't testing that. And we're still missing a recvonly test. > That makes me wonder if should either rename this test case or > create a separate test case specifically for sendonly only. Well, I think this one tests mozCapture first and foremost. I guess it couldn't hurt to test sendonly gUM as well, though I'm no stickler for discrete tests either. Others would have to comment on whether they think it would be superfluous. > Maybe you should add an ok(stream.getTracks().length >= 1, "...") before > this to prevent that this loop does nothing and you are left with just the > info message above. Probably you could replace the info() with an ok(). Good idea. I'll do: is(stream.getTracks().length, 2, "Captured stream has 2 tracks"); which is almost the same, since the quantity is known. Please yell if you disagree. > > + test.pcLocal.constraints = [{ video: true, audio:true }]; // fool tests > > Is this still needed even with patch 2? Yes, the test-harness seems to be built around several assumptions, and one is that all sources come from gUM. E.g. the verifySdp code in peerConnectionWrapper uses constraints to "count" how many tracks to expect. > If so shouldn't we fix it or at least file a bug for it for later? I'm happy to leave that to you. :-) While I have some ideas about the test framework, they're fairly radical. > > + ok(false, "unexpected failure: " + reason); > > + SimpleTest.finish(); > > On a local manual test run from command line this works, on build/test > servers this unfortunately not necessarily exits the test early (I'm > assuming that's what you want here). Not sure what you mean (you have an extra 'not' in there). Can you explain another way what the local vs. builder difference is? My intent here is to exit test_peerConnection_capturedVideo.html only.

Updated commit msg. Carrying forward r=jesup.

Incorporated feedback. Carrying forward r=drno.

Incorporated feedback. Thanks! Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bb544c3db785

https://hg.mozilla.org/integration/mozilla-inbound/rev/69200484dcd5 https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6c695178df https://hg.mozilla.org/integration/mozilla-inbound/rev/a26d802146be

https://hg.mozilla.org/mozilla-central/rev/69200484dcd5 https://hg.mozilla.org/mozilla-central/rev/9f6c695178df https://hg.mozilla.org/mozilla-central/rev/a26d802146be