Add a bunch of basic functional tests for getUserMedia and enable a per config setting for those tests for fake vs. non-fake

RESOLVED FIXED in mozilla21

Status

()

Core
WebRTC
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jsmith, Assigned: jsmith)

Tracking

Trunk
mozilla21
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia][blocking-gum-][qa-])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
Huge dump of automated tests for gum along with the per config setting to allow you to change one value to switch from fake to real devices for the gum tests only. After I land this, I'll port over the peer connection tests to this config as well.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 820007
(Assignee)

Comment 2

5 years ago
Created attachment 692764 [details] [diff] [review]
gUM BFT and config setting - v1

Actually, I'll do the peerconnection conversion now.
(Assignee)

Comment 3

5 years ago
Comment on attachment 692764 [details] [diff] [review]
gUM BFT and config setting - v1

Feedback first for a few reasons:

1. Needs a try run. Odds are some of these are going to pass locally, but fail in CI. I currently don't have try access (although that's my fault, as I still need to complete the contributor form...)

2. Not ready for review just yet as there's some very minor code nits to still cleanup. After the try run with the results, I'll stick up a followup patch based on the results + address the nits I know about
Attachment #692764 - Flags: feedback?(hskupin)
Comment on attachment 692764 [details] [diff] [review]
gUM BFT and config setting - v1

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

Overall it looks good! There is one thing we have to figure out for peer connection tests but see my comment inline for the appropriate tests. Otherwise mostly styling issues.

::: dom/media/tests/mochitest/Makefile.in
@@ +26,5 @@
>    test_getUserMedia_basicAudio.html \
>    test_getUserMedia_basicVideoAudio.html \
> +  test_getUserMedia_gumWithinGum.html \
> +  test_getUserMedia_playVideoTwice.html \
> +  test_getUserMedia_playAudioTwice.html \

nit: please move that up so it's alphabetically ordered.

@@ +29,5 @@
> +  test_getUserMedia_playVideoTwice.html \
> +  test_getUserMedia_playAudioTwice.html \
> +  test_getUserMedia_playVideoAudioTwice.html \
> +  test_getUserMedia_stopVideoStream.html \
> +  test_getUserMedia_stopAudioStream.html \

same here.

@@ +32,5 @@
> +  test_getUserMedia_stopVideoStream.html \
> +  test_getUserMedia_stopAudioStream.html \
> +  test_getUserMedia_stopVideoAudioStream.html \
> +  test_getUserMedia_stopVideoStreamWithFollowupVideo.html \
> +  test_getUserMedia_stopAudioStreamWithFollowupAudio.html \

and here.

::: dom/media/tests/mochitest/head.js
@@ +5,5 @@
>  var Cc = SpecialPowers.Cc;
>  var Ci = SpecialPowers.Ci;
>  var Cr = SpecialPowers.Cr;
>  
> +var fakeEnabled = true;

Please make that a constant which also includes capital letters. Also a comment wouldn't be that bad right next to its declaration.

@@ +26,5 @@
>      ok(true, navigator.userAgent + ' currently not supported');
>      SimpleTest.finish();
>    } else {
> +    SpecialPowers.pushPrefEnv({'set': [['media.peerconnection.enabled', true],
> +      ['media.navigator.permission.disabled', true]]}, aCallback);

Mind wrapping the first line so that the first pref aligns to the second one? Would make it easier to check.

Tests which will exercise the notification path will have to reset this pref in any case within the runTest() callback.

::: dom/media/tests/mochitest/mediaStreamPlayback.js
@@ +22,5 @@
>    /**
>     * Starts the media with the associated stream.
>     *
> +   * @param {Boolean} isResume specifies if the media element playback
> +   *                           is being resumed from a previous run

Can't we store that boolean in the instance? Why would we have to use it as a parameter for all the functions?

@@ +41,5 @@
> +
> +    // If we're initially running this media, check that the time is zero
> +    if(!isResume) {
> +      is(this.mediaStream.currentTime, 0,
> +        "Before starting the media element, currentTime = 0");

Usually we align with the first letter after the opening bracket. But just for the future...

@@ +156,5 @@
>    /**
> +   * Stops the local media stream while it's currently in playback in
> +   * a media element.
> +   *
> +   * @param {Integer} timeoutLength the length of time to wait for certain

nit: There is no 'Integer' type in JS. What you want is number.

@@ +165,5 @@
> +   *                           fails to fire an ended event from stop() being
> +   *                           called
> +   *
> +   * Precondition: The media stream and element should both be actively
> +   *               being played.

Could you please move this up before the @param lines? It's important to know and can be overseen at this position.

@@ +167,5 @@
> +   *
> +   * Precondition: The media stream and element should both be actively
> +   *               being played.
> +   */
> +  this.stopStreamInMediaPlayback = function(timeoutLength, onSuccess, onError) {

Please keep an alphabetical order of the helper functions in this method.

@@ +187,5 @@
> +    this.mediaStream.stop();
> +
> +    // If ended doesn't fire in enough time, then we fail the test
> +    setTimeout(function() {
> +      if(!endedFired) {

nit: Please put a blank between 'if' and the opening bracket. Same applies to function. That's happening multiple times here in this module.

@@ +237,5 @@
> +    var self = this;
> +
> +    this.startMedia(isResume, timeoutLength, function() {
> +      self.stopStreamInMediaPlayback(timeoutLength, function() {
> +        self.stopMedia();

stopMedia is a bit confusing here. Can we call it stopMediaElement?

::: dom/media/tests/mochitest/test_getUserMedia_basicAudio.html
@@ +31,3 @@
>        var testAudio = document.getElementById('testAudio');
>        var audioStreamPlayback = new MediaStreamPlayback(testAudio, stream);
> +      audioStreamPlayback.playMedia(false, 10000, function() {

Why the increase from 5.000 to 10.000 milliseconds? I haven't seen failures with a timeout of 5s so far.

::: dom/media/tests/mochitest/test_getUserMedia_exceptions.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Please fix your editor so it doesn't include this BOM header in most of the files of this patch.

::: dom/media/tests/mochitest/test_getUserMedia_gumWithinGum.html
@@ +12,5 @@
> +  <script type="application/javascript" src="mediaStreamPlayback.js"></script>
> +</head>
> +<body>
> +<video id="testVideo"></video>
> +<audio id="testAudio"></audio>

both video and audio elements have to live inside the content div. This comment applies to all the gum tests.

@@ +33,5 @@
> +      var testVideo = document.getElementById('testVideo');
> +      var videoStreamPlayback = new MediaStreamPlayback(testVideo, videoStream);
> +      videoStreamPlayback.playMedia(false, 10000, function() {
> +
> +        getUserMedia({audio: true},

nit: To make this easier to read you should add the blank line after the variable declarations and before the call to playMedia() but not inside the callback.

@@ +37,5 @@
> +        getUserMedia({audio: true},
> +          function(audioStream) {
> +            var testAudio = document.getElementById('testAudio');
> +            var audioStreamPlayback = new MediaStreamPlayback(testAudio,
> +              audioStream);

That's a weird indentation I have never seen before. If you wrap the line the first letter of the following line should be aligned with with opening bracket or at least with 'new'.

@@ +48,5 @@
> +      }, unexpectedCallbackAndFinish);
> +    }, unexpectedCallbackAndFinish);
> +  } catch (err) {
> +    unexpectedCallbackAndFinish(err);
> +  }

nit: broken indentation for all the last 5 lines.

::: dom/media/tests/mochitest/test_getUserMedia_stopAudioStreamWithFollowupAudio.html
@@ +35,5 @@
> +        getUserMedia({audio: true},
> +          function(secondStream) {
> +          var secondStreamPlayback = new MediaStreamPlayback(testAudio,
> +            secondStream);
> +          secondStreamPlayback.playMedia(true, 10000, function() {

please fix the indentation.

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoAudioStreamWithFollowupVideoAudio.html
@@ +35,5 @@
> +        getUserMedia({video: true, audio: true},
> +          function(secondStream) {
> +          var secondStreamPlayback = new MediaStreamPlayback(testVideo,
> +            secondStream);
> +          secondStreamPlayback.playMedia(true, 10000, function() {

please fix the indentation.

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStreamWithFollowupVideo.html
@@ +35,5 @@
> +        getUserMedia({video: true},
> +          function(secondStream) {
> +          var secondStreamPlayback = new MediaStreamPlayback(testVideo,
> +            secondStream);
> +          secondStreamPlayback.playMedia(true, 10000, function() {

please fix the indentation.

@@ +41,5 @@
> +            SimpleTest.finish();
> +          }, unexpectedCallbackAndFinish);
> +        }, unexpectedCallbackAndFinish);
> +      }, unexpectedCallbackAndFinish);
> +    }, unexpectedCallbackAndFinish);

For code blocks like those blank lines at the right spot would make it easier to read.

::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html
@@ +63,3 @@
>        pcLocal.addStream(aLocalInputStream);
>  
> +      getUserMedia({audio: true}, function onSuccess(aRemoteInputStream) {

I'm not sure if that works here! It would require you to have two real audio devices available. So the second call would have to remain a faked stream, at least as long we can simulate devices.

::: dom/media/tests/mochitest/test_peerConnection_basicVideo.html
@@ +64,3 @@
>        pcLocal.addStream(aLocalInputStream);
>  
> +      getUserMedia({video: true}, function onSuccess(aRemoteInputStream) {

Same as for the other peer connection test.
Attachment #692764 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Updated

5 years ago
Assignee: nobody → jsmith
(Assignee)

Comment 5

5 years ago
Henrik - Can you also do a try run on this patch? I'd be curious to see what the results are to find out problems with the test, actual bugs, what we need to pref on vs. not, etc...
Flags: needinfo?(hskupin)
Pushed to try. Please check later:
https://tbpl.mozilla.org/?tree=Try&rev=3fdd3e2cdb8a
Flags: needinfo?(hskupin)
Whiteboard: [blocking-gum+]
Jason, when you land the gUM tests, can you open a new bug (dependent on this one) for PeerConnection?  Thanks.
(Assignee)

Updated

5 years ago
Whiteboard: [blocking-gum+] → [getUserMedia][blocking-gum+]
(In reply to Maire Reavy [:mreavy] from comment #7)
> Jason, when you land the gUM tests, can you open a new bug (dependent on
> this one) for PeerConnection?  Thanks.

This is bug 795383. But I don't see why it has to be a dependency.
(Assignee)

Comment 9

5 years ago
Flipping this to non-blocking - the only piece of automation that I think blocks for preffing on is any automation that is smoke tests for the feature. We should definitely do this, but if push came to shove, I still think we would pref on without this finished.
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum-]
(Assignee)

Updated

5 years ago
Attachment #692764 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 708886 [details] [diff] [review]
gUM BFT v2

Finally got time to finish this off.
(Assignee)

Updated

5 years ago
Attachment #708886 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 708887 [details] [diff] [review]
gUM BFT v2
(Assignee)

Updated

5 years ago
Attachment #708887 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 708888 [details] [diff] [review]
gUM BFT v2
(Assignee)

Comment 13

5 years ago
Comment on attachment 708888 [details] [diff] [review]
gUM BFT v2

Fixed most of the stuff brought up in the patch and cleaned up other misc stuff too.
Attachment #708888 - Flags: review?(rjesup)
Attachment #708888 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Blocks: 749522
Comment on attachment 708888 [details] [diff] [review]
gUM BFT v2

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

All in all it looks good and some nice improvements are in this patch. But looks like further work is necessary until it's complete. See my comments inline.

::: dom/media/tests/mochitest/head.js
@@ +6,5 @@
>  var Ci = SpecialPowers.Ci;
>  var Cr = SpecialPowers.Cr;
>  
> +// Specifies whether we are using fake streams to run this automation
> +var FAKE_ENABLED = true;

I'm not sure what's best here. As variable as used here or as an env variable you can read via SpecialPowers. The latter wouldn't require to make changes to this file for testing and once faked devices are available we could easily setup default values through the build process.

I would like to hear Randell's or Eric's opinion on it.

@@ +51,5 @@
> + * @param {Function} onError the error callback if the stream fails to be
> + *                           retrieved
> + */
> +function getUserMedia(constraints, onSuccess, onError) {
> +  constraints["fake"] = FAKE_ENABLED;

That would cause us to always use faked or non-faked streams. Can we ensure that for a larger test we can request enough streams from the faked devices? If not it might break future tests.

::: dom/media/tests/mochitest/mediaStreamPlayback.js
@@ +20,2 @@
>  
> +MediaStreamPlayback.prototype = {

So I see that this is my code from bug 833144. Good to see that it made it in.

@@ -34,5 @@
>      var canPlayThroughFired = false;
>  
> -    // Verifies we've received a correctly initialized LocalMediaStream
> -    ok(this.mediaStream instanceof LocalMediaStream,
> -      "Stream should be a LocalMediaStream");

You might want to leave this check but change it to check for an instance of MediaStream.

@@ +185,3 @@
>  
>    /**
> +   * Stops the local media stream while it's currently in playback in

nit: this is not a local media stream.

@@ +212,5 @@
>        onSuccess();
> +    };
> +
> +    this.mediaElement.addEventListener('ended', endedCallback, false);
> +    this.mediaStream.stop();

.stop() is only available for localMediaStream objects. So it can't be used in this class. You have to move it to the new LocalMediaStreamPlayback class.

@@ +252,5 @@
> + * @param {Function} onError the error function call back
> + *                           if media fails to start
> + */
> +LocalMediaStreamPlayback.prototype.startMedia = function(isResume, onSuccess,
> +  onError) {

nit: When you wrap a list of parameters try to let the next line start in the same column as the first parameter the line before.

::: dom/media/tests/mochitest/test_getUserMedia_exceptions.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

I think you added again a BOM header to this file.

::: dom/media/tests/mochitest/test_getUserMedia_gumWithinGum.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

nit: BOM header again, which applies to all of the newly added files. Please fix that.

@@ +29,5 @@
> +  runTest(function () {
> +    getUserMedia({video: true}, function(videoStream) {
> +      var testVideo = document.getElementById('testVideo');
> +      var videoStreamPlayback = new LocalMediaStreamPlayback(testVideo,
> +        videoStream);

nit: As mentioned earlier, please try to wrap correctly when you want to keep the 80 char limit. As example see:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/head.js#1215

::: dom/media/tests/mochitest/test_getUserMedia_playAudioTwice.html
@@ +31,5 @@
> +
> +      audioStreamPlayback.playMedia(false, function() {
> +
> +        audioStreamPlayback.playMedia(true, function() {
> +          audioStream.stop();

So the mozSrcObject gets reset in-between both of those tests. Shouldn't we better leave it alone so we can really resume? Or how does the video audio element react on when you remove the stream?

::: dom/media/tests/mochitest/test_getUserMedia_playVideoAudioTwice.html
@@ +30,5 @@
> +
> +      streamPlayback.playMedia(false, function() {
> +
> +        streamPlayback.playMedia(true, function() {
> +          stream.stop();

Same as for the audio test.

::: dom/media/tests/mochitest/test_getUserMedia_playVideoTwice.html
@@ +31,5 @@
> +
> +      videoStreamPlayback.playMedia(false, function() {
> +
> +        videoStreamPlayback.playMedia(true, function() {
> +          videoStream.stop();

Same as for the other two.

::: dom/media/tests/mochitest/test_getUserMedia_stopAudioStreamWithFollowupAudio.html
@@ +32,5 @@
> +        firstStream);
> +
> +      firstStreamPlayback.playMediaWithStreamStop(false, function() {
> +        getUserMedia({audio: true}, function(secondStream) {
> +          var secondStreamPlayback = new LocalMediaStreamPlayback(testAudio,

Instead of creating another instance of LocalMediaStreamPlayback we might consider to add a public stream property to the class, so we can re-use the former instance and really continue where we left of. With a new instance you might initialize the media element in the future and resuming would not be possible without having to update all the tests. Also it would remove a dependency and we could keep track of the resume parameter inside of the playback class. This also applies to the other tests with this combination.

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoAudioStreamWithFollowupVideoAudio.html
@@ +20,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +  /**
> +   * Run a test to verify that I can complete an audio gum playback in a media

This is about audio and video. Most likely you missed that to update when c&p the lines.

nit: can you please remove the reference to 'I'?

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStream.html
@@ +27,5 @@
> +  runTest(function () {
> +    getUserMedia({video: true}, function(stream) {
> +      var testVideo = document.getElementById('testVideo');
> +      var videoStreamPlayback = new LocalMediaStreamPlayback(testVideo, stream);
> +      videoStreamPlayback.playMediaWithStreamStop(false, SimpleTest.finish,

nit: Mind adding an empty line before for readiness?

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStreamWithFollowupVideo.html
@@ +20,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +  /**
> +   * Run a test to verify that I can complete an audio gum playback in a media

Same as before with the comment.
Attachment #708888 - Flags: review?(hskupin) → review-
(Assignee)

Comment 15

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #14)
> Comment on attachment 708888 [details] [diff] [review]
> gUM BFT v2
> 
> Review of attachment 708888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All in all it looks good and some nice improvements are in this patch. But
> looks like further work is necessary until it's complete. See my comments
> inline.
> 
> ::: dom/media/tests/mochitest/head.js
> @@ +6,5 @@
> >  var Ci = SpecialPowers.Ci;
> >  var Cr = SpecialPowers.Cr;
> >  
> > +// Specifies whether we are using fake streams to run this automation
> > +var FAKE_ENABLED = true;
> 
> I'm not sure what's best here. As variable as used here or as an env
> variable you can read via SpecialPowers. The latter wouldn't require to make
> changes to this file for testing and once faked devices are available we
> could easily setup default values through the build process.
> 
> I would like to hear Randell's or Eric's opinion on it.

Originally I was trying to get something in place here to at least allow for local testing against real streams. But it wasn't something really intended to ever be used with SpecialPowers, however.

> 
> @@ +51,5 @@
> > + * @param {Function} onError the error callback if the stream fails to be
> > + *                           retrieved
> > + */
> > +function getUserMedia(constraints, onSuccess, onError) {
> > +  constraints["fake"] = FAKE_ENABLED;
> 
> That would cause us to always use faked or non-faked streams. Can we ensure
> that for a larger test we can request enough streams from the faked devices?
> If not it might break future tests.

I don't understand. The intention here is a control flow to allow for local testing against real streams. This doesn't have anything to do with the fake device flow yet.

> 
> ::: dom/media/tests/mochitest/mediaStreamPlayback.js
> @@ +20,2 @@
> >  
> > +MediaStreamPlayback.prototype = {
> 
> So I see that this is my code from bug 833144. Good to see that it made it
> in.
> 
> @@ -34,5 @@
> >      var canPlayThroughFired = false;
> >  
> > -    // Verifies we've received a correctly initialized LocalMediaStream
> > -    ok(this.mediaStream instanceof LocalMediaStream,
> > -      "Stream should be a LocalMediaStream");
> 
> You might want to leave this check but change it to check for an instance of
> MediaStream.

Okay, will fix.

> 
> @@ +185,3 @@
> >  
> >    /**
> > +   * Stops the local media stream while it's currently in playback in
> 
> nit: this is not a local media stream.

Will fix.

> 
> @@ +212,5 @@
> >        onSuccess();
> > +    };
> > +
> > +    this.mediaElement.addEventListener('ended', endedCallback, false);
> > +    this.mediaStream.stop();
> 
> .stop() is only available for localMediaStream objects. So it can't be used
> in this class. You have to move it to the new LocalMediaStreamPlayback class.

Will fix.

> 
> @@ +252,5 @@
> > + * @param {Function} onError the error function call back
> > + *                           if media fails to start
> > + */
> > +LocalMediaStreamPlayback.prototype.startMedia = function(isResume, onSuccess,
> > +  onError) {
> 
> nit: When you wrap a list of parameters try to let the next line start in
> the same column as the first parameter the line before.

Err...style wise, I think it's the other way. Usually you want:

 thisisafunc(param1, param2,
    param3, param4);

Not:

 thisisafunc(param1, param2,
             param3, param4);

> 
> ::: dom/media/tests/mochitest/test_getUserMedia_exceptions.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> I think you added again a BOM header to this file.

I switched the encoding from ANSI to UTF-8. So that's probably why you are seeing that.

> 
> ::: dom/media/tests/mochitest/test_getUserMedia_gumWithinGum.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> nit: BOM header again, which applies to all of the newly added files. Please
> fix that.

I switched the encoding from ANSI to UTF-8. So that's probably why you are seeing that.

> 
> @@ +29,5 @@
> > +  runTest(function () {
> > +    getUserMedia({video: true}, function(videoStream) {
> > +      var testVideo = document.getElementById('testVideo');
> > +      var videoStreamPlayback = new LocalMediaStreamPlayback(testVideo,
> > +        videoStream);
> 
> nit: As mentioned earlier, please try to wrap correctly when you want to
> keep the 80 char limit. As example see:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/browser/head.js#1215

See my comment above on why I don't necessarily agree with that.

> 
> ::: dom/media/tests/mochitest/test_getUserMedia_playAudioTwice.html
> @@ +31,5 @@
> > +
> > +      audioStreamPlayback.playMedia(false, function() {
> > +
> > +        audioStreamPlayback.playMedia(true, function() {
> > +          audioStream.stop();
> 
> So the mozSrcObject gets reset in-between both of those tests. Shouldn't we
> better leave it alone so we can really resume? Or how does the video audio
> element react on when you remove the stream?

The element does react when you remove it I believe (the media element would be affected by the stream being removed, as there wouldn't be content to stream anymore). I'd actually prefer to keep a cleanup approach instead that I already have in that case.

> 
> ::: dom/media/tests/mochitest/test_getUserMedia_playVideoAudioTwice.html
> @@ +30,5 @@
> > +
> > +      streamPlayback.playMedia(false, function() {
> > +
> > +        streamPlayback.playMedia(true, function() {
> > +          stream.stop();
> 
> Same as for the audio test.

See comment above.

> 
> ::: dom/media/tests/mochitest/test_getUserMedia_playVideoTwice.html
> @@ +31,5 @@
> > +
> > +      videoStreamPlayback.playMedia(false, function() {
> > +
> > +        videoStreamPlayback.playMedia(true, function() {
> > +          videoStream.stop();
> 
> Same as for the other two.

See comment above.

> 
> :::
> dom/media/tests/mochitest/test_getUserMedia_stopAudioStreamWithFollowupAudio.
> html
> @@ +32,5 @@
> > +        firstStream);
> > +
> > +      firstStreamPlayback.playMediaWithStreamStop(false, function() {
> > +        getUserMedia({audio: true}, function(secondStream) {
> > +          var secondStreamPlayback = new LocalMediaStreamPlayback(testAudio,
> 
> Instead of creating another instance of LocalMediaStreamPlayback we might
> consider to add a public stream property to the class, so we can re-use the
> former instance and really continue where we left of. With a new instance
> you might initialize the media element in the future and resuming would not
> be possible without having to update all the tests. Also it would remove a
> dependency and we could keep track of the resume parameter inside of the
> playback class. This also applies to the other tests with this combination.

I'm not exactly sure I entirely follow. There's already a variable for the stream object on MediaStreamPlayback object. Once this function is called, the stream really can't be used for playback anymore. 

I'll investigate what I can do with the variable already specified.

> 
> :::
> dom/media/tests/mochitest/
> test_getUserMedia_stopVideoAudioStreamWithFollowupVideoAudio.html
> @@ +20,5 @@
> > +<pre id="test">
> > +<script type="application/javascript">
> > +
> > +  /**
> > +   * Run a test to verify that I can complete an audio gum playback in a media
> 
> This is about audio and video. Most likely you missed that to update when
> c&p the lines.
> 
> nit: can you please remove the reference to 'I'?

Will fix.

> 
> ::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStream.html
> @@ +27,5 @@
> > +  runTest(function () {
> > +    getUserMedia({video: true}, function(stream) {
> > +      var testVideo = document.getElementById('testVideo');
> > +      var videoStreamPlayback = new LocalMediaStreamPlayback(testVideo, stream);
> > +      videoStreamPlayback.playMediaWithStreamStop(false, SimpleTest.finish,
> 
> nit: Mind adding an empty line before for readiness?

Will fix.

> 
> :::
> dom/media/tests/mochitest/test_getUserMedia_stopVideoStreamWithFollowupVideo.
> html
> @@ +20,5 @@
> > +<pre id="test">
> > +<script type="application/javascript">
> > +
> > +  /**
> > +   * Run a test to verify that I can complete an audio gum playback in a media
> 
> Same as before with the comment.

Will fix.
(Assignee)

Comment 16

5 years ago
My notes for what I'm fixing:

- Add instanceof check for MediaStream
- Fix comments in MediaStreamPlayback - reference MediaStream, not LocalMediaStream
- Move stop() functions to LocalMediaStreamPlayback
- Don't recreate objects, try to set a new stream if possible
- Fix comment in stopVideoAudioStreamWithFollowupVideoAudio & stopVideoStreamWithFollowupVideo
- New line before playMediaWithStreamStop in stopVideoStream
(Assignee)

Updated

5 years ago
Attachment #708888 - Flags: review?(rjesup)
(In reply to Jason Smith [:jsmith] from comment #15)
> Originally I was trying to get something in place here to at least allow for
> local testing against real streams. But it wasn't something really intended
> to ever be used with SpecialPowers, however.

See my next comment.

> > > +function getUserMedia(constraints, onSuccess, onError) {
> > > +  constraints["fake"] = FAKE_ENABLED;
> > 
> > That would cause us to always use faked or non-faked streams. Can we ensure
> > that for a larger test we can request enough streams from the faked devices?
> > If not it might break future tests.
> 
> I don't understand. The intention here is a control flow to allow for local
> testing against real streams. This doesn't have anything to do with the fake
> device flow yet.

Well, as what I remember is that we will not test with faked media streams once the faked devices are available. Is that not the case? I believe Randell and Eric can clarify that.

If that would be the case we need a flag to switch between faked streams and faked devices, and that would most likely be the same flag. It will be set conditionally depending on the platform and support for faked devices.

Until then I'm not sure tests especially the peer connection tests will pass with that flag set to false. You will need multiple audio and video devices connected to the get the number of streams required by the handshake. Do those tests pass for you with only a single camera and microphone connected?

> > > +LocalMediaStreamPlayback.prototype.startMedia = function(isResume, onSuccess,
> > > +  onError) {
> > 
> > nit: When you wrap a list of parameters try to let the next line start in
> > the same column as the first parameter the line before.
> 
> Err...style wise, I think it's the other way. Usually you want:
> 
>  thisisafunc(param1, param2,
>     param3, param4);
> 
> Not:
> 
>  thisisafunc(param1, param2,
>              param3, param4);

The style you are proposing here is not the one we make use of at Mozilla. If you don't agree, please check MXR for other JS files.

> > ::: dom/media/tests/mochitest/test_getUserMedia_playAudioTwice.html
> > @@ +31,5 @@
> > > +
> > > +      audioStreamPlayback.playMedia(false, function() {
> > > +
> > > +        audioStreamPlayback.playMedia(true, function() {
> > > +          audioStream.stop();
> > 
> > So the mozSrcObject gets reset in-between both of those tests. Shouldn't we
> > better leave it alone so we can really resume? Or how does the video audio
> > element react on when you remove the stream?
> 
> The element does react when you remove it I believe (the media element would
> be affected by the stream being removed, as there wouldn't be content to
> stream anymore). I'd actually prefer to keep a cleanup approach instead that
> I already have in that case.

In this case I don't see the requirement of the test fulfilled. You want to resume in the second case, which states 'true' as first parameter. But given that we null out the media object and re-attach it to the video element, it will not be a true resume. The initial time value is 0 again, right?

> > > +      firstStreamPlayback.playMediaWithStreamStop(false, function() {
> > > +        getUserMedia({audio: true}, function(secondStream) {
> > > +          var secondStreamPlayback = new LocalMediaStreamPlayback(testAudio,
> > 
> > Instead of creating another instance of LocalMediaStreamPlayback we might
> > consider to add a public stream property to the class, so we can re-use the
> > former instance and really continue where we left of. With a new instance
> > you might initialize the media element in the future and resuming would not
> > be possible without having to update all the tests. Also it would remove a
> > dependency and we could keep track of the resume parameter inside of the
> > playback class. This also applies to the other tests with this combination.
> 
> I'm not exactly sure I entirely follow. There's already a variable for the
> stream object on MediaStreamPlayback object. Once this function is called,
> the stream really can't be used for playback anymore. 

Can you please explain why it cannot be re-used anymore for playback? I don't understand this, especially because we also have the resume case where we attach a stream again to the media element. Here we have a different stream but for the media element it wouldn't make any difference. 

So what I meant is:

firstStreamPlayback.stream = secondStream;
firstStreamPlayback.playMediaWithStreamStop(..)
Status: NEW → ASSIGNED
Flags: needinfo?(rjesup)
(Assignee)

Comment 18

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #15)
> > Originally I was trying to get something in place here to at least allow for
> > local testing against real streams. But it wasn't something really intended
> > to ever be used with SpecialPowers, however.
> 
> See my next comment.
> 
> > > > +function getUserMedia(constraints, onSuccess, onError) {
> > > > +  constraints["fake"] = FAKE_ENABLED;
> > > 
> > > That would cause us to always use faked or non-faked streams. Can we ensure
> > > that for a larger test we can request enough streams from the faked devices?
> > > If not it might break future tests.
> > 
> > I don't understand. The intention here is a control flow to allow for local
> > testing against real streams. This doesn't have anything to do with the fake
> > device flow yet.
> 
> Well, as what I remember is that we will not test with faked media streams
> once the faked devices are available. Is that not the case? I believe
> Randell and Eric can clarify that.
> 
> If that would be the case we need a flag to switch between faked streams and
> faked devices, and that would most likely be the same flag. It will be set
> conditionally depending on the platform and support for faked devices.
> 
> Until then I'm not sure tests especially the peer connection tests will pass
> with that flag set to false. You will need multiple audio and video devices
> connected to the get the number of streams required by the handshake. Do
> those tests pass for you with only a single camera and microphone connected?

You are definitely misinterpreting something here.

1) The PC tests aren't modified to use this. Only the gUM tests
2) This was meant for local testing with real devices
3) Your scope creeping this patch. This doesn't target anything to do with faked devices yet. Please save that for this patch.

Removing needsinfo as there's no modification needed here and this is a misinterpretation.

> 
> > > > +LocalMediaStreamPlayback.prototype.startMedia = function(isResume, onSuccess,
> > > > +  onError) {
> > > 
> > > nit: When you wrap a list of parameters try to let the next line start in
> > > the same column as the first parameter the line before.
> > 
> > Err...style wise, I think it's the other way. Usually you want:
> > 
> >  thisisafunc(param1, param2,
> >     param3, param4);
> > 
> > Not:
> > 
> >  thisisafunc(param1, param2,
> >              param3, param4);
> 
> The style you are proposing here is not the one we make use of at Mozilla.
> If you don't agree, please check MXR for other JS files.

Just because mxr shows multiple examples doesn't say that's right.

Looking this one up, here's what I found - https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style:

Don't restrict yourself to 80-character lines. Google's Android style guide suggests 100-character lines. Java code tends to be long horizontally, so use appropriate judgement when wrapping. Avoid deep indents on wrapping. Note that aligning the wrapped part of a line with some previous part of the line (rather than just using a fixed indent) may require shifting the code every time the line changes, resulting in spurious whitespace changes.

Although this is Java, the point applies generally. That's actually why you usually lean towards avoiding parameter mapping as it leads to overuse of whitespace and multiple unnecessary lines. 

Reading http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Multi-level_prototype_hierarchies as a point of comparison indicates either approach is fine, although that's a different style guideline not for this codebase.

So I'm not convinced that you are right here. I don't ever have seen a cut and dry suggestion that this is *always* a good style choice.

> 
> > > ::: dom/media/tests/mochitest/test_getUserMedia_playAudioTwice.html
> > > @@ +31,5 @@
> > > > +
> > > > +      audioStreamPlayback.playMedia(false, function() {
> > > > +
> > > > +        audioStreamPlayback.playMedia(true, function() {
> > > > +          audioStream.stop();
> > > 
> > > So the mozSrcObject gets reset in-between both of those tests. Shouldn't we
> > > better leave it alone so we can really resume? Or how does the video audio
> > > element react on when you remove the stream?
> > 
> > The element does react when you remove it I believe (the media element would
> > be affected by the stream being removed, as there wouldn't be content to
> > stream anymore). I'd actually prefer to keep a cleanup approach instead that
> > I already have in that case.
> 
> In this case I don't see the requirement of the test fulfilled. You want to
> resume in the second case, which states 'true' as first parameter. But given
> that we null out the media object and re-attach it to the video element, it
> will not be a true resume. The initial time value is 0 again, right?

No. It would not be zero. That's would be why the isResume piece was introduced. So I don't think you are right here.

> 
> > > > +      firstStreamPlayback.playMediaWithStreamStop(false, function() {
> > > > +        getUserMedia({audio: true}, function(secondStream) {
> > > > +          var secondStreamPlayback = new LocalMediaStreamPlayback(testAudio,
> > > 
> > > Instead of creating another instance of LocalMediaStreamPlayback we might
> > > consider to add a public stream property to the class, so we can re-use the
> > > former instance and really continue where we left of. With a new instance
> > > you might initialize the media element in the future and resuming would not
> > > be possible without having to update all the tests. Also it would remove a
> > > dependency and we could keep track of the resume parameter inside of the
> > > playback class. This also applies to the other tests with this combination.
> > 
> > I'm not exactly sure I entirely follow. There's already a variable for the
> > stream object on MediaStreamPlayback object. Once this function is called,
> > the stream really can't be used for playback anymore. 
> 
> Can you please explain why it cannot be re-used anymore for playback? I
> don't understand this, especially because we also have the resume case where
> we attach a stream again to the media element. Here we have a different
> stream but for the media element it wouldn't make any difference. 
> 
> So what I meant is:
> 
> firstStreamPlayback.stream = secondStream;
> firstStreamPlayback.playMediaWithStreamStop(..)

Once you call stop() on a stream you aren't going to be able to play the stream again. It's a dead-stop function. You would need to recall gUM to get another stream from the camera and do as you've specified above.
Flags: needinfo?(rjesup)
(Assignee)

Comment 19

5 years ago
(In reply to Jason Smith [:jsmith] from comment #18)
> (In reply to Henrik Skupin (:whimboo) from comment #17)
> > (In reply to Jason Smith [:jsmith] from comment #15)
> > > Originally I was trying to get something in place here to at least allow for
> > > local testing against real streams. But it wasn't something really intended
> > > to ever be used with SpecialPowers, however.
> > 
> > See my next comment.
> > 
> > > > > +function getUserMedia(constraints, onSuccess, onError) {
> > > > > +  constraints["fake"] = FAKE_ENABLED;
> > > > 
> > > > That would cause us to always use faked or non-faked streams. Can we ensure
> > > > that for a larger test we can request enough streams from the faked devices?
> > > > If not it might break future tests.
> > > 
> > > I don't understand. The intention here is a control flow to allow for local
> > > testing against real streams. This doesn't have anything to do with the fake
> > > device flow yet.
> > 
> > Well, as what I remember is that we will not test with faked media streams
> > once the faked devices are available. Is that not the case? I believe
> > Randell and Eric can clarify that.
> > 
> > If that would be the case we need a flag to switch between faked streams and
> > faked devices, and that would most likely be the same flag. It will be set
> > conditionally depending on the platform and support for faked devices.
> > 
> > Until then I'm not sure tests especially the peer connection tests will pass
> > with that flag set to false. You will need multiple audio and video devices
> > connected to the get the number of streams required by the handshake. Do
> > those tests pass for you with only a single camera and microphone connected?
> 
> You are definitely misinterpreting something here.
> 
> 1) The PC tests aren't modified to use this. Only the gUM tests
> 2) This was meant for local testing with real devices
> 3) Your scope creeping this patch. This doesn't target anything to do with
> faked devices yet. Please save that for this patch.
> 
> Removing needsinfo as there's no modification needed here and this is a
> misinterpretation.

Meant to say "Save this for a different patch"
(In reply to Jason Smith [:jsmith] from comment #18)
> You are definitely misinterpreting something here.
> 
> 1) The PC tests aren't modified to use this. Only the gUM tests
> 2) This was meant for local testing with real devices
> 3) Your scope creeping this patch. This doesn't target anything to do with
> faked devices yet. Please save that for this patch.

So if that's the scope for now, I'm fine with. But you should be prepared that we have to update that code again once faked devices are available. So for now it would be good if you would update the name of the variable and make it a bit clearer.

> Just because mxr shows multiple examples doesn't say that's right.
> 
> So I'm not convinced that you are right here. I don't ever have seen a cut
> and dry suggestion that this is *always* a good style choice.

As I have said you will have a hard time to find any instance in the js files which will apply to your suggested style. I will not argument longer on that style and leave it for the review from Randell. 

> > In this case I don't see the requirement of the test fulfilled. You want to
> > resume in the second case, which states 'true' as first parameter. But given
> > that we null out the media object and re-attach it to the video element, it
> > will not be a true resume. The initial time value is 0 again, right?
> 
> No. It would not be zero. That's would be why the isResume piece was
> introduced. So I don't think you are right here.

playMedia() calls startMedia() which itself will fire the onSuccess callback once succeeded. Inside the callback you call stopMediaElement() which first pauses the stream and then sets the mozSrcObject of the media element to null. Here we loose the connectivity between the media element and the stream, and that's why I wonder about the time value of the media element when you attach a stream the next time.

> > Can you please explain why it cannot be re-used anymore for playback? I
> > don't understand this, especially because we also have the resume case where
> > we attach a stream again to the media element. Here we have a different
> > stream but for the media element it wouldn't make any difference. 
> > 
> > So what I meant is:
> > 
> > firstStreamPlayback.stream = secondStream;
> > firstStreamPlayback.playMediaWithStreamStop(..)
> 
> Once you call stop() on a stream you aren't going to be able to play the
> stream again. It's a dead-stop function. You would need to recall gUM to get
> another stream from the camera and do as you've specified above.

Correct, and I do not say anything against that. I simply wonder if we can make it easier by allowing to attach a new stream to the already existent instance of MediaStreamPlayback without having to create a new one all the time.

While saying that please make sure you also cleanup the created instances of MediaStreamPlayback so it will not be left for the GC.
(Assignee)

Comment 21

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #18)
> > You are definitely misinterpreting something here.
> > 
> > 1) The PC tests aren't modified to use this. Only the gUM tests
> > 2) This was meant for local testing with real devices
> > 3) Your scope creeping this patch. This doesn't target anything to do with
> > faked devices yet. Please save that for this patch.
> 
> So if that's the scope for now, I'm fine with. But you should be prepared
> that we have to update that code again once faked devices are available. So
> for now it would be good if you would update the name of the variable and
> make it a bit clearer.

I think FAKE_ENABLED is pretty clear name. I don't see a problem here.

> 
> > > In this case I don't see the requirement of the test fulfilled. You want to
> > > resume in the second case, which states 'true' as first parameter. But given
> > > that we null out the media object and re-attach it to the video element, it
> > > will not be a true resume. The initial time value is 0 again, right?
> > 
> > No. It would not be zero. That's would be why the isResume piece was
> > introduced. So I don't think you are right here.
> 
> playMedia() calls startMedia() which itself will fire the onSuccess callback
> once succeeded. Inside the callback you call stopMediaElement() which first
> pauses the stream and then sets the mozSrcObject of the media element to
> null. Here we loose the connectivity between the media element and the
> stream, and that's why I wonder about the time value of the media element
> when you attach a stream the next time.

That's probably a worthwhile question to ask Roc.

> 
> > > Can you please explain why it cannot be re-used anymore for playback? I
> > > don't understand this, especially because we also have the resume case where
> > > we attach a stream again to the media element. Here we have a different
> > > stream but for the media element it wouldn't make any difference. 
> > > 
> > > So what I meant is:
> > > 
> > > firstStreamPlayback.stream = secondStream;
> > > firstStreamPlayback.playMediaWithStreamStop(..)
> > 
> > Once you call stop() on a stream you aren't going to be able to play the
> > stream again. It's a dead-stop function. You would need to recall gUM to get
> > another stream from the camera and do as you've specified above.
> 
> Correct, and I do not say anything against that. I simply wonder if we can
> make it easier by allowing to attach a new stream to the already existent
> instance of MediaStreamPlayback without having to create a new one all the
> time.

That's fine. 

> 
> While saying that please make sure you also cleanup the created instances of
> MediaStreamPlayback so it will not be left for the GC.

I don't think that needs to be relevant to any of these tests. The fact that they are going through the GC or not I don't think really matters.
(Assignee)

Comment 22

5 years ago
(In reply to Jason Smith [:jsmith] from comment #21)
> 
> > 
> > > > In this case I don't see the requirement of the test fulfilled. You want to
> > > > resume in the second case, which states 'true' as first parameter. But given
> > > > that we null out the media object and re-attach it to the video element, it
> > > > will not be a true resume. The initial time value is 0 again, right?
> > > 
> > > No. It would not be zero. That's would be why the isResume piece was
> > > introduced. So I don't think you are right here.
> > 
> > playMedia() calls startMedia() which itself will fire the onSuccess callback
> > once succeeded. Inside the callback you call stopMediaElement() which first
> > pauses the stream and then sets the mozSrcObject of the media element to
> > null. Here we loose the connectivity between the media element and the
> > stream, and that's why I wonder about the time value of the media element
> > when you attach a stream the next time.
> 
> That's probably a worthwhile question to ask Roc.

Wait, I know the answer to my own question. If we're re-attaching an existing stream that was used before to start playing it again, the time would not be zero - the time would actually be where we left off when we paused. That's why that check was introduced - some of these tests aim to check that case of playing the stream once, yank it from the media element and pause it, and play it again. We need to make the stream represents a true resume.
Flags: needinfo?(roc)
(Assignee)

Comment 23

5 years ago
Opps, put need info by accident.
Flags: needinfo?(roc)
(Assignee)

Updated

5 years ago
Attachment #708888 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 711189 [details] [diff] [review]
gUM BFT v3
(Assignee)

Comment 25

5 years ago
Comment on attachment 711189 [details] [diff] [review]
gUM BFT v3

This addresses the things we're in agreement on.
Attachment #711189 - Flags: review?(hskupin)
(In reply to Jason Smith [:jsmith] from comment #22)
> Wait, I know the answer to my own question. If we're re-attaching an
> existing stream that was used before to start playing it again, the time
> would not be zero - the time would actually be where we left off when we
> paused. That's why that check was introduced - some of these tests aim to
> check that case of playing the stream once, yank it from the media element
> and pause it, and play it again. We need to make the stream represents a
> true resume.

Hm, so that might only apply to local mediastreams which buffer the data. For remote streams we don't do this AFAIR. Given that the specific code in question is located in the MediaStreamPlayback class I would like to hear what's the expected behavior is.
(Assignee)

Comment 27

5 years ago
(In reply to Henrik Skupin (:whimboo) [away 02/09 - 02/017] from comment #26)
> (In reply to Jason Smith [:jsmith] from comment #22)
> > Wait, I know the answer to my own question. If we're re-attaching an
> > existing stream that was used before to start playing it again, the time
> > would not be zero - the time would actually be where we left off when we
> > paused. That's why that check was introduced - some of these tests aim to
> > check that case of playing the stream once, yank it from the media element
> > and pause it, and play it again. We need to make the stream represents a
> > true resume.
> 
> Hm, so that might only apply to local mediastreams which buffer the data.
> For remote streams we don't do this AFAIR. Given that the specific code in
> question is located in the MediaStreamPlayback class I would like to hear
> what's the expected behavior is.

Local media streams don't buffer data, but the time can continuously pass on the object still.

I think this discussion making me realize that we just need the ability to do both options for now, which this patch allows for.

I'll let Roc maybe shed some light on this discussion cause I don't understand what's isn't clear.

And this time I'm settings needsinfo correctly...
Flags: needinfo?(roc)
Stream playback will continue whether it's connected to a media element or not --- unless it's connected to a media element that is paused. In that case, the stream will pause (even a gUM stream). If it ever unpauses, all the content between the pause time and the unpause time is effectively thrown away. Make sense?
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Stream playback will continue whether it's connected to a media element or
> not --- unless it's connected to a media element that is paused. In that

Thanks Roc, but my initial question was the other way around. How does the media element react when you null out the stream object. Is the time getting reset or does it keep the state and return when the next stream (even the same) is getting attached via mozSrcObject?
The element's currentTime will be reset, just like if you reloaded the video by resetting the "src" attribute to a media file.
(Assignee)

Comment 31

5 years ago
Comment on attachment 711189 [details] [diff] [review]
gUM BFT v3

With all these questions, I think it's a good idea to get your review on this patch. That might help reduce the open questions here.
Attachment #711189 - Flags: review?(hskupin) → review?(roc)
Comment on attachment 711189 [details] [diff] [review]
gUM BFT v3

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

I don't want to sort through this bug to find out what the issues are. Please tell me what you need to know.
Attachment #711189 - Flags: review?(roc)
(Assignee)

Comment 33

5 years ago
Comment on attachment 711189 [details] [diff] [review]
gUM BFT v3

Actually, I figured out an answer to my own question. There is an error in the patch based on Henrik's points. I'll fix it.
Attachment #711189 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Created attachment 711626 [details] [diff] [review]
gUM BFT v4

So I fixed the tests titled "with followup" to use false twice. That's probably what you were talking about with the stream getting reset. Now it does the time check as you were mentioning.
Attachment #711626 - Flags: review?(hskupin)
Comment on attachment 711626 [details] [diff] [review]
gUM BFT v4

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

This looks way better now, and seems to be testing the right stuff. There is one thing we might wanna do in a follow-up bug. See my comment inline. Otherwise please ask Randell for review and let him give his opinion about the coding style. I still don't support it, but if he is ok with it, it's fine.

r=me if tests on try are passing:
https://tbpl.mozilla.org/?tree=Try&rev=9a572de0a449

::: dom/media/tests/mochitest/mediaStreamPlayback.js
@@ +54,5 @@
>      var self = this;
>      var canPlayThroughFired = false;
>  
> +    // If we're initially running this media, check that the time is zero
> +    if (!isResume) {

So right now we will never hit this case in the current tests. I think it's worth creating a follow-up bug which can cover a real resume for audio, video, and audio+video. With that we might want to introduce a setter for the mediastream property so that the class can handle the resume flag internally.
Attachment #711626 - Flags: review?(hskupin) → review+
(Assignee)

Comment 36

5 years ago
(In reply to Henrik Skupin (:whimboo) [away 02/09 - 02/017] from comment #35)
> Comment on attachment 711626 [details] [diff] [review]
> gUM BFT v4
> 
> Review of attachment 711626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks way better now, and seems to be testing the right stuff. There is
> one thing we might wanna do in a follow-up bug. See my comment inline.
> Otherwise please ask Randell for review and let him give his opinion about
> the coding style. I still don't support it, but if he is ok with it, it's
> fine.
> 
> r=me if tests on try are passing:
> https://tbpl.mozilla.org/?tree=Try&rev=9a572de0a449
> 
> ::: dom/media/tests/mochitest/mediaStreamPlayback.js
> @@ +54,5 @@
> >      var self = this;
> >      var canPlayThroughFired = false;
> >  
> > +    // If we're initially running this media, check that the time is zero
> > +    if (!isResume) {
> 
> So right now we will never hit this case in the current tests. I think it's
> worth creating a follow-up bug which can cover a real resume for audio,
> video, and audio+video. With that we might want to introduce a setter for
> the mediastream property so that the class can handle the resume flag
> internally.

Actually, we do hit this case in the current tests. Take a look at the tests titled playAudioTwice, playVideoTwice, and playVideoAudioTwice. 

      audioStreamPlayback.playMedia(false, function() {

        audioStreamPlayback.playMedia(true, function() {

That's where the purpose of that check comes in. If I made both of these false, then the second time I called playMedia would fail (and yes, I confirmed that was true when cleaning up the patch).

I'll wait for comments on Randell here as well.

So two items to raise to Randell are:

- JS Style
- Resume approach for the playMediaTwice tests
(Assignee)

Comment 37

5 years ago
Comment on attachment 711626 [details] [diff] [review]
gUM BFT v4

Existing try run pieces that have finished look good (and I'm thinking it will be clean).

Randell - Can you do the peer review of the gUM tests and address also specifically the two items in the above comment?
Attachment #711626 - Flags: review?(rjesup)
(In reply to Jason Smith [:jsmith] from comment #36)
> Actually, we do hit this case in the current tests. Take a look at the tests
> titled playAudioTwice, playVideoTwice, and playVideoAudioTwice. 
> 
>       audioStreamPlayback.playMedia(false, function() {
> 
>         audioStreamPlayback.playMedia(true, function() {
> 
> That's where the purpose of that check comes in. If I made both of these
> false, then the second time I called playMedia would fail (and yes, I
> confirmed that was true when cleaning up the patch).

Hm, given Roc's feedback I wonder if that is a bug then. We reset mozSrcObject and reattach it in the second call, so as I understood the time should be zero again.
When the source is a stream, we make mediaElement.currentTime return the currentTime of the stream, which isn't necessarily 0 if the stream has been playing already. This isn't really specified anywhere but I think that behavior is logical.
Comment on attachment 711626 [details] [diff] [review]
gUM BFT v4

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

I'm fine with the indent style here; since JS tends to use closures like embedded functions as arguments, preserving indent space is a valuable tool.  In C++, it's a more open question, and while I prefer aligned-with-previous-params I've been forced by reviewers in some files to go the other way.  I don't are much except that within a file it should be consistent.

::: dom/media/tests/mochitest/test_getUserMedia_exceptions.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Even looking at the diff, I can't see a difference here...
Attachment #711626 - Flags: review?(rjesup) → review+
(Assignee)

Comment 41

5 years ago
(In reply to Randell Jesup [:jesup] from comment #40)
> Comment on attachment 711626 [details] [diff] [review]
> gUM BFT v4
> 
> Review of attachment 711626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm fine with the indent style here; since JS tends to use closures like
> embedded functions as arguments, preserving indent space is a valuable tool.
> In C++, it's a more open question, and while I prefer
> aligned-with-previous-params I've been forced by reviewers in some files to
> go the other way.  I don't are much except that within a file it should be
> consistent.
> 
> ::: dom/media/tests/mochitest/test_getUserMedia_exceptions.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> Even looking at the diff, I can't see a difference here...

Oh. I changed the Content Type from ANSI to UTF-8, since all of the files checked in here except this one were UTF-8. So that's the difference you are seeing here.

Try run is green, got a r+ from jesup and whimboo, so this is ready to be checked in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e3a4589e251
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Updated

5 years ago
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][qa-]
You need to log in before you can comment on or make changes to this bug.