898952 - Media Recording - Avoid to record a stopped MediaStream

Status: VERIFIED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

A stopped MediaStream can't be resumed and record-able. So we need t patch to prevent mediaRecorder record such MediaStream.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

Fix wrong comment

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

try result: 
https://tbpl.mozilla.org/?tree=Try&rev=d0e1c369fd5f

Comment 4

[Jason Smith [:jsmith]]

Probably doesn't hurt to include a mochitest here btw. Something as simple as setting up a media stream like the mediarecorder creation test, calling stop on the stream, and starting recording. Verify an exception gets thrown here.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

umm, this need the gUM's fake audio ready
Bug 895730 - MediaRecorder - Can't get any encoded data from encoder when recording the mediaStream with mute audio
Because gUM's localMediaStream has the stop method.

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case

test case for this issue.
try: https://tbpl.mozilla.org/?tree=Try&rev=bbc875e09bff

Comment 7

[Jason Smith [:jsmith]]

Comment on attachment 784168 [details] [diff] [review]
test case

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

review- for cleanup & setTimeout usage.

::: content/media/test/test_mediarecorder_recordstopms.html
@@ +10,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {
> +
> +  navigator.mozGetUserMedia({audio:true,fake:true},

gUM right now is only supported on Desktop Firefox & Firefox for Android, so we'll miss out on Firefox OS coverage until 1.2 gUM support lands on B2G.

I think we'll benefit better if we use the gMediaRecorderTests flow like what's seen in test_mediarecorder_creation.html

@@ +15,5 @@
> +                       function(s) {
> +                         s.stop();
> +                         mr = new MediaRecorder(s);
> +                         // wait the mediaStream stop for a while
> +                         setTimeout(function() {

I don't think you need a setTimeout function here - remove this.

@@ +28,5 @@
> +                       },
> +                       function(e) {dump(e)});
> +}
> +
> +startTest();

It would actually be better to use the gMediaRecorderTests workflow here instead of using a specialized test here not using the framework - that would get more coverage when we add new mime types to gMediaRecorderTests.

manager.runTests(gMediaRecorderTests, startTest);

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Ok, sound reasonable. Could you help to add this test into gMediaRecorderTests also? :)

Comment 9

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #8)
> Ok, sound reasonable. Could you help to add this test into
> gMediaRecorderTests also? :)

I could, although this should be a quick fix to implement this if you would like to finish this off. You just need to use manager.runTests(gMediaRecorderTests, startTest) to start the test, do the typical element setup you see in mediarecorder_creation, stop the stream, and try to start recording with that stopped stream. Verify that an exception gets thrown.

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The mediaStream comes from mozCaptureStreamUntilEnded doesn't support the stop function, 
So I can't use this to test it.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The localmediastream's stop method is async, so I delay 1s for waiting the mediastream state enter endded.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #10)
> The mediaStream comes from mozCaptureStreamUntilEnded doesn't support the
> stop function, 
> So I can't use this to test it.

Why not try using mozCaptureStream then?

(In reply to Randy Lin [:rlin] from comment #11)
> The localmediastream's stop method is async, so I delay 1s for waiting the
> mediastream state enter endded.

You could get around doing a timeout here by attaching the stream under test to a media element and waiting for onended to fire on the element after stop is called.

General note - It's usually best to avoid using setTimeout in automation, mainly because it can cause tests to run unreliably in continuous integration.

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The mozCaptureStream return the MediaStream interface, not the localmediastream object.
BTW, I will try to find a better way to test this. Thanks for your information. :)

Comment 14

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #13)
> The mozCaptureStream return the MediaStream interface, not the
> localmediastream object.
> BTW, I will try to find a better way to test this. Thanks for your
> information. :)

Ah, that's right. So in that case, we have to use gUM then. So ignore my comments then about using gMediaRecorder tests.

Here's what you need to do instead then:

* Get rid of the setTimeout call
* Use the media element's onended event handler to know stop() was called on the stream
* Then call start in the onended event handler with stopped stream
* Verify you get an exception

Note - since gUM is not enabled on b2g yet, you'll see to add this test to the b2g.json file as disabled test. We can reenable it once B2G gUM support lands.

Comment 15

[Jason Smith [:jsmith]]

Btw - b2g.json is here - http://hg.mozilla.org/mozilla-central/file/05d3797276d3/testing/mochitest/b2g.json.

Comment 16

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case

try to stop the audio tag and get a stopped mediastream.

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 784435 [details] [diff] [review]
test case

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

If this invokes the exception in this bug, then this looks good with one hit. Only giving feedback+ cause I'm less knowledgeable about if this mochitest triggers the behavior we expect, so I'm sending the review over to roc to confirm.

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +19,5 @@
> +  element.src = test.name;
> +  element.test = test;
> +  element.stream = element.mozCaptureStreamUntilEnded();
> +  var mediaRecorder = new MediaRecorder(element.stream);
> +    try {

Nit - indentation is off here with the try block.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #17)
> Comment on attachment 784435 [details] [diff] [review]
> test case
> 
> Review of attachment 784435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If this invokes the exception in this bug, then this looks good with one
> hit. Only giving feedback+ cause I'm less knowledgeable about if this
> mochitest triggers the behavior we expect, so I'm sending the review over to
> roc to confirm.

That should be nit, not hit.

Comment 19

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 784435 [details] [diff] [review]
test case

Catch wrong exception, let me check check
throw by element.currentTime = 0;

Comment 20

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case

Test case for capture this bug.
Use two audio element and try to avoid the latency of media stream isFinished status chagned.

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

try server 
https://tbpl.mozilla.org/?tree=Try&rev=5cd4f7ee8a84

Comment 22

[Jason Smith [:jsmith]]

Comment on attachment 784835 [details] [diff] [review]
test case

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

The concept of the test looks okay, but I'm confused why two media elements are needed here.

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> +  element2.src = test.name;
> +  element2.onended = function() {
> +    element.play();
> +  }
> +  element2.play();

Why do you need two media elements here?

@@ +28,5 @@
> +  element.test = test;
> +
> +  element.onended = function() {
> +    element.mr = new MediaRecorder(element.stream);
> +    element.mr.ondataavailable = function(e) {};

You shouldn't need this for what you are aiming to test in this mochitest.

@@ +32,5 @@
> +    element.mr.ondataavailable = function(e) {};
> +    try {
> +      element.mr.start(1000);
> +      ok(false, 'Should catch exception and block recording');
> +      manager.finished(token);

Nit - you can cut down the code duplication of two manager.finished(token) calls by introducing a finally block.

try {

} catch(e) {

} finally {

}

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I found the media stream isn't stopped during the audio element onended event. 
And It seems no event for notifying mediastream has been stopped. 
So I use this way to avoid this async problem.

Comment 24

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #23)
> I found the media stream isn't stopped during the audio element onended
> event. 
> And It seems no event for notifying mediastream has been stopped. 
> So I use this way to avoid this async problem.

Odd. I'm starting to think that we might want to just to go back to fake gUM stream approach.

Comment 25

[Jason Smith [:jsmith]]

Attachment: (Untested) Stopped gUM Stream Mochitest Approach

Here's an untested demonstration of what I'm talking about.

Comment 26

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I think both approach is fine for this bug. :)

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 784835 [details] [diff] [review]
test case

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

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> +  element2.src = test.name;
> +  element2.onended = function() {
> +    element.play();
> +  }
> +  element2.play();

I agree, we should be able to just play an element through until it's ended, then create a MediaRecorder for its stream and start() it.

Comment 28

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case

use only one audio element to catch this bug.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 785626 [details] [diff] [review]
test case

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

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +21,5 @@
> +
> +  element.onended = function() {
> +    element.play();
> +    element.onended = function() {
> +      element.mr = new MediaRecorder(element.stream);

Why are you playing the element through twice? Once should be enough.

Comment 30

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Is any way to capture the mediaStream isFinished event? I found if I just try to record the mediaStream after onended event, check the mediaStream status and found it isn't IsFinished() already, should wait a while...

Comment 31

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case v3

Change to use fake gUM to get the stopped media stream.
try result
https://tbpl.mozilla.org/?tree=Try&rev=80b6445514ae

Comment 32

[Jason Smith [:jsmith]]

Comment on attachment 786159 [details] [diff] [review]
test case v3

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

Concept of the test looks okay, although you need to get rid of the setTimeout call.

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> + */
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {
> +    stream.stop();
> +    setTimeout(function () {

This shouldn't be needed. We should avoid using setTimeout in tests as it leads to inconsistent test results.

Comment 33

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Return the original problem, The media stream stop method is async, 
Hi Roc, do we have a better way to identify the media stream has entered a stopped state?

Comment 34

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #33)
> Return the original problem, The media stream stop method is async, 
> Hi Roc, do we have a better way to identify the media stream has entered a
> stopped state?

Hmm...you could create a media element in the test, attach the gUM stream to that element, play it, set an onended handler, and call stop on the stream. When onended fires, you should be able to confirm that the stream has stopped.

Comment 35

[Jason Smith [:jsmith]]

Attachment: Concept using onended

Comment 36

[Jason Smith [:jsmith]]

Attachment: Concept using onended

Comment 37

[Jason Smith [:jsmith]]

See the attached concept. That's one way you can guarantee the stream is definitely stopped.

Comment 38

[Jason Smith [:jsmith]]

Btw - don't forget to pref this off in the testing/mochitest/b2g.json file. We currently don't have gUM enabled on Firefox OS, so this mochitest will only be able to be ran on Desktop & Android until gUM gets enabled on Firefox OS.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randy Lin [:rlin] from comment #33)
> Return the original problem, The media stream stop method is async, 
> Hi Roc, do we have a better way to identify the media stream has entered a
> stopped state?

You could create a new media element, set its srcObject to the stream, and wait for "ended" to fire on the media element.

Comment 40

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Thanks roc and Jason, I will modify it :)

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh I see Jason beat me to it in comment #34. Sorry :-)

Comment 42

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v4

Thanks Jason's test code. It works. 
try record:
https://tbpl.mozilla.org/?tree=Try&rev=ea16005446c6

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 786748 [details] [diff] [review]
patch v4

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

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {
> +    var testAudio = document.getElementById('testAudio');
> +
> +    testAudio.oncanplaythrough = function() {
> +      stream.stop();

I think you should call stream.stop() right after calling testAudio.play().

Comment 44

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v5

Follow by roc's suggestion, 
try 
https://tbpl.mozilla.org/?tree=Try&rev=031be29a1092

Comment 45

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: disable test on b2g

By comment 14, 
gUM is not enabled on b2g, disable test first.

Comment 46

[Jason Smith [:jsmith]]

Comment on attachment 786806 [details] [diff] [review]
disable test on b2g

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

::: testing/mochitest/b2g.json
@@ +20,5 @@
>      "content/media/test/test_bug686942.html": "timed out",
>      "content/media/test/test_can_play_type.html":"timed out",
>      "content/media/test/test_can_play_type_mpeg.html":"7 failures out of 27",
>      "content/media/test/test_can_play_type_no_dash.html":"",
> +    "content/media/test/test_mediarecord_stopped_stream.html":"",

This isn't the name of the mochitest you've created. The name of the mochitest you've created is:

test_mediarecorder_record_stopms.html

Comment 47

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: disable b2g test

Comment 48

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch 1

check-in patch 1, carry reviewer.

Comment 49

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch 2

check in patch 2, carry reviewer.

Comment 50

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch 2

check-in patch, carry reviewer

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/86ed8b6613f2
https://hg.mozilla.org/integration/b2g-inbound/rev/f30d4621eac8

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86ed8b6613f2
https://hg.mozilla.org/mozilla-central/rev/f30d4621eac8

Comment 53

[Jason Smith [:jsmith]]

Verified via landing of mochitest with no backouts.

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

> https://hg.mozilla.org/mozilla-central/rev/f30d4621eac8

Is a bug filed for this failure? Can you list the bug number here?

Comment 55

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

From comment 14, the gUM isn't enabled on b2g, so avoid this this test first.

Comment 56

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Randy Lin [:rlin] from comment #55)
> From comment 14, the gUM isn't enabled on b2g, so avoid this this test first.

I was asking for a bug, not an explanation.

Comment 57

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I created new one for tracking this one.
Bug 903765 - Media Recording - enable mochitest which used the mozGetUserMedia on b2g.