The currently existing Mochitests for peer connection should be updated to check for the media flow. We can do that by verifying the timestamp and that the content is changing at least for video with different colors. Not sure yet if we have test data for audio which can be also used. Randell, Eric?

I think Fake audio is still silence, but I'm not totally sure. Have you turned on the speaker and seen fi you get anything?

Henrik - I believe you were working on this, right? If so, what's blocking you from finishing this? Also, if you have a partially completed patch here, can you stick up a WIP here?

Stealing with whimboo - he's focused on data channel automation and this along with that automation has been deemed as high priority.

Comment on attachment 732676 [details] [diff] [review] Check for media flow in PC tests v1 Short, simple patch. I took advantage of the work I did on the gUM automation MediaStreamPlayback framework here to reuse all of the media checks we already have.

Comment on attachment 732676 [details] [diff] [review] Check for media flow in PC tests v1 Small grammar fix need to make on the patch, let me get an updated patch here...

Comment on attachment 732680 [details] [diff] [review] Check for media flow in PC tests v1 Review of attachment 732680 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/pc.js @@ +476,5 @@ > + mediaStreamPlayback.startMedia(false, function() { > + ok(true, "Media stream (" + type + ", " + side + ") is running"); > + }, function() { > + ok(false, "Media stream (" + type + ", " + side + ") did not start"); > + }); The messages here will be added with an unknown delay to the log output. This can cause variations by running the code several times due to the async nature. Depending on the following code we want to execute this can even cause failures. So I propose we wait until startMedia() has been finished. We might have to create another command for that task. ::: dom/media/tests/mochitest/test_peerConnection_bug827843.html @@ +4,5 @@ > <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > <script type="application/javascript" src="head.js"></script> > <script type="application/javascript" src="pc.js"></script> > + <script type="application/javascript" src="mediaStreamPlayback.js"></script> I'm not that happy with that dependency. I wonder if mochitests support `require()` so the pc.js module can load that other js module itself.

(In reply to Henrik Skupin (:whimboo) from comment #9) > Comment on attachment 732680 [details] [diff] [review] > Check for media flow in PC tests v1 > > Review of attachment 732680 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/tests/mochitest/pc.js > @@ +476,5 @@ > > + mediaStreamPlayback.startMedia(false, function() { > > + ok(true, "Media stream (" + type + ", " + side + ") is running"); > > + }, function() { > > + ok(false, "Media stream (" + type + ", " + side + ") did not start"); > > + }); > > The messages here will be added with an unknown delay to the log output. > This can cause variations by running the code several times due to the async > nature. Depending on the following code we want to execute this can even > cause failures. So I propose we wait until startMedia() has been finished. > We might have to create another command for that task. A way we can make this a little more deterministic would be to add success/error callbacks to attachMedia and fire a success callback when we finish successfully, error callback otherwise. The second problem needing to be solved here would require us modifying setRemoteDescription's command to only move onto the next command only after startMedia finishes. I need to think about this one a little bit more. > > ::: dom/media/tests/mochitest/test_peerConnection_bug827843.html > @@ +4,5 @@ > > <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > > <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > > <script type="application/javascript" src="head.js"></script> > > <script type="application/javascript" src="pc.js"></script> > > + <script type="application/javascript" src="mediaStreamPlayback.js"></script> > > I'm not that happy with that dependency. I wonder if mochitests support > `require()` so the pc.js module can load that other js module itself. I don't think this is a big deal. This was going to happen eventually and makes sense that two JS modules were going to build on each other. I'd rather not get into a style analysis here on these details.

So here's an idea that I think might work: By default, onaddstream is setup with a callback to fire a failure when we don't expect it to fire within a command. So the PC Wrapper constructor would do something like: this._pc.onaddstream = function(event) { ok(false, "Unexpected onaddstream callback"); }; Now, let's say we expect a command will result in onaddstream firing. In the command, I'd change it to only execute the next command after the expected callbacks fires. Meaning an example like this (note - not perfectly valid code, but just demonstrating concept here): function (test) { var onaddstreamfired = false; test.pcRemote.SetOnAddStream(function(event) { self.attachMedia(event.stream, 'video', 'remote', function() { if(!onaddstreamfired) { onaddstreamfired = true; test.next(); } else { ok(false, "Unexpected onaddstream callback"); } }, function() { SimpleTest.finish(); }); }); test.pcRemote.setRemoteDescription(test.pcLocal._last_offer, function () { }); } How about that?

One added thing actually - we would need to reset the onaddstream callback before next is called too.

A second idea would be the following: We would start with the same idea as the first: this._pc.onaddstream = function(event) { ok(false, "Unexpected onaddstream callback"); }; But change something slightly to setup multiple "expectations" that need to be met before moving on to the next command. In the set remote description case, we are expecting the success callback and attachMedia to be successful. This means the following: function (test) { var onaddstreamfired = false; var setRemoteDescFinished = false; function isFinished() { if(onaddstreamfired && setRemoteDescFinished) { test.next(); } } test.pcRemote.SetOnAddStream(function(event) { self.attachMedia(event.stream, 'video', 'remote', function() { if(!onaddstreamfired) { onaddstreamfired = true; isFinished(); } else { ok(false, "Unexpected onaddstream callback"); } }, function() { SimpleTest.finish(); }); }); test.pcRemote.setRemoteDescription(test.pcLocal._last_offer, function () { setRemoteDescFinished = true; isFinished(); }); } This approach guarantees all callbacks we're expecting will fire, but I'm wondering if this is safe or not to do.

Comment on attachment 732940 [details] [diff] [review] Experimental Approach 1 Found out why I was having trouble getting this working. I hit the issue whimboo talking about exceptions getting eaten - there was a syntax error in the patch.

So I've attached an update experimental approach I'm considering. However, when I run this, I end up seeing the canplaythrough event failing to fire on media elements derived from remote streams: 353 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudio.html | canplaythrough event never fired Strangely enough, my local reduced test case is working fine. Randell - Any ideas? Is this a bug in development code or the test?

From my experience toying around with the WebRTC APIs, I've seen this behavior of the canplaythrough event failing to fire when the remote stream provided through peers is broken. But what's causing it be broken in this case?

I've filed bug 859195 for the issue I'm hitting. Clearing needinfo and moving the discussion to the dependent bug.

Just found over the weekend. Please have a look at the following URL which describes how we could check for a real media flow for video. That way we could proof that the remote video is correctly playing. http://timtaubert.de/blog/2013/02/getusermedia-part-3-simple-motion-detection-in-a-live-video/

(In reply to Henrik Skupin (:whimboo) from comment #20) > Just found over the weekend. Please have a look at the following URL which > describes how we could check for a real media flow for video. That way we > could proof that the remote video is correctly playing. > > http://timtaubert.de/blog/2013/02/getusermedia-part-3-simple-motion- > detection-in-a-live-video/ That's certainly a different way to solve the problem and a different possibility for validation. For simplicity, I'd file a different bug for that enhancement though - the basis of media flow checking is already defined in mediaStreamPlayback, although there's always more enhancements we can do here.

Eric explained what the problem is here - media flow only starts happening after the handshake is complete, not mid-way. So right now, the approach attached will end up in deadlock.

Good news is the attached patch resolves the deadlock issue I hit over in bug 859195. Bad news is that I'm still failing to get canplaythrough to fire on a media element with a remote media stream when analyzed after the peer connection handshake is established. Talking with Eric, he thinks this outside of peer connection land and now into the land of media streams being the root cause (whether that be a bug in the test framework vs. a bug in media streams). Roc - Can you give some input here on why you think canplaythrough isn't firing?

I don't know. How do I run your testcase?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > I don't know. How do I run your testcase? 1. Import the attached patch 2. Build the dom/media/tests/mochitest directory 3. In the base mozilla central directory, run mach mochitest-plain dom/media/tests/mochitest/test_peerConnection_basicAudio.html What you'll notice when you run this is you'll see the two media elements playing, but we'll get hung waiting for the canplaythrough event to fire on the media element with the remote media stream.

I did that. It seems to me that in pc.js, checkMedia sets a canplaythrough listener on the first element, and waits for canplaythrough to fire on that element and then a timeupdate step on that element. Then it adds a canplaythrough listener on the next element --- but canplaythrough has already fired on that element! You can tell because the element's readyState is HAVE_ENOUGH_DATA, but you don't check the readyState. So this looks like a bug in pc.js to me.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > I did that. It seems to me that in pc.js, checkMedia sets a canplaythrough > listener on the first element, and waits for canplaythrough to fire on that > element and then a timeupdate step on that element. Then it adds a > canplaythrough listener on the next element --- but canplaythrough has > already fired on that element! You can tell because the element's readyState > is HAVE_ENOUGH_DATA, but you don't check the readyState. So this looks like > a bug in pc.js to me. Ah, makes sense. Thanks for the input. I'll rework the patch as such then.

The attached patch demonstrates a new concept that takes Roc's feedback into account. For cases we are expecting media flow to be present, this concept appears to work in a robust fashion over multiple runs. This won't work in the case where we are expecting to have no media flow present, however. That seems to be the case with our media constraint tests, which makes sense, since we are specified constraints but we have no media being sent between the two peers.

Here's an updated concept that works for checking when we expect media flow and when we do not expect media flow. Now that I've got the concept nailed down, I'll go cleanup the code to get an actual patch up for review here.

Comment on attachment 754296 [details] [diff] [review] Check for Media Flow in PC Tests review on Randell for landing, but including roc for feedback to check that the way I'm doing media flow checking makes sense in the PC mochitests.

Looks OK to me.

Comment on attachment 754296 [details] [diff] [review] Check for Media Flow in PC Tests Review of attachment 754296 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/head.js @@ +202,5 @@ > + var e = new Error(); > + ok(false, "Unexpected event '" + eventName + "' fired for " + description + > + " " + e.stack.split("\n")[1]); > + SimpleTest.finish(); > + } This collides with my patch on bug 796894 for datachannel, which implemented this first. So you might want to depend on it.

Comment on attachment 754296 [details] [diff] [review] Check for Media Flow in PC Tests I'll take Roc's comment above as a feedback+ then.

Comment on attachment 754296 [details] [diff] [review] Check for Media Flow in PC Tests Review of attachment 754296 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/head.js @@ +202,5 @@ > + var e = new Error(); > + ok(false, "Unexpected event '" + eventName + "' fired for " + description + > + " " + e.stack.split("\n")[1]); > + SimpleTest.finish(); > + } It's the same code from your patch with nothing changed except the above comment above the function. I actually think we should go the other direction and land this first for two reasons: 1. This isn't blocked by any bug to land outside of getting Randell's review 2. I think merging this with your patch will be easier done if this lands first instead of the other direction

Backed out for mochitest-3 timeouts. Try kthxbye. https://hg.mozilla.org/integration/mozilla-inbound/rev/a116d4a86539 https://tbpl.mozilla.org/php/getParsedLog.php?id=23864852&tree=Mozilla-Inbound

Hmm...onaddstream appears to be failing to fire.

Comment on attachment 760558 [details] [diff] [review] Media Flow Checking in PC Tests Review of attachment 760558 [details] [diff] [review]: ----------------------------------------------------------------- Updated patch to address removing the onaddstream check in setRemoteDescription and made a few misc fixes.

All green on try. Should be good to land with a quick pass over review by Randell.

Please be aware that the current patch will no longer apply cleanly anymore due to the additions made by my patch on bug 881658.

(In reply to Henrik Skupin (:whimboo) from comment #49) > Please be aware that the current patch will no longer apply cleanly anymore > due to the additions made by my patch on bug 881658. I think I already indicated earlier that I think it would be better to land this first and rebase your patch against this patch.

Jason, you mix up bugs. I don't talk about the datachannel test framework. As indicated this patch needs an update and cannot be checked-in as it is right now.

Comment on attachment 775478 [details] [diff] [review] Rebased - PC Media Flow Checking Rebased with the Data Channel changes. Asking for review to double check rebase happened cleanly. Note - the changes to the constraint tests are no longer included due to bug 893649. There's a regression on trunk that causes onaddstream to fail to fire, so I do not have an ability to test the constraint tests on trunk rebased against the latest changes. When bug 893649 gets fixed, we can land on a separate patch (probably as part of the bug) to re-enable the media flow tests for the constraint tests. I'm also running into trouble with my try build pushing, so I have only verified this works locally so far.

Comment on attachment 775478 [details] [diff] [review] Rebased - PC Media Flow Checking Actually, I'm going to change my mind. I'm going to include the full set of changes in this patch.

Comment on attachment 776137 [details] [diff] [review] Rebased - PC Media Flow Checking Upon talking with Ethan, turns out the changes here for the media constraint tests aren't right. I'm going back to the patch I had before as a result.

Finally got my try access back, so here's the try run kicked off: https://tbpl.mozilla.org/?tree=Try&rev=146c0252ace4

Try run appears to be green.